media: Add MojoDecoderBuffer{Reader|Writer}

media: Add MojoDecoderBuffer{Reader|Writer}

This removes duplicate code in clients/services that transfer/receive
media DecoderBuffers over DataPipe.

The implementation of *::ConvertDecoderBuffer() is
copied from MojoAudioDecoder*, which has recently
been updated and reviewed by mojo owners
(https://chromiumcodereview.appspot.com/1982883003/)

BUG=621784
TEST=Moved unittest to a new file with improvements.

Committed: https://crrev.com/1087d4ca6be621c1e687987b40df73c27783a800
Cr-Commit-Position: refs/heads/master@{#402496}

[xhwang, 2016-06-24 19:04:49]

Description was changed from

==========
media: Add MojoDecoderBuffer{Reader|Writer}

This removes duplicate code in clients/services that transfer/receive
media DecoderBuffers over DataPipe.

BUG=621784
TEST=Moved unittest to a new file with improvements.
==========

to

==========
media: Add MojoDecoderBuffer{Reader|Writer}

This removes duplicate code in clients/services that transfer/receive
media DecoderBuffers over DataPipe.

The implementation of *::ConvertDecoderBuffer() is
copied from MojoAudioDecoder*, which has recently
been updated and reviewed by mojo owners
(https://chromiumcodereview.appspot.com/1982883003/)

BUG=621784
TEST=Moved unittest to a new file with improvements.
==========

[xhwang, 2016-06-24 19:04:49]

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

[xhwang, 2016-06-24 19:05:44]

sandersd, alokp: PTAL

timav, jrummell, rockot: FYI

[alokp, 2016-06-24 19:35:10]

This looks pretty good. I just have high-level comment on API and function
names.

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/clients/m...
File media/mojo/clients/mojo_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/clients/m...
media/mojo/clients/mojo_audio_decoder.cc:166: mojo_decoder_buffer_writer_ =
MojoDecoderBufferWriter::Create(
To keep things clear and flexible I would suggest letting clients of
MojoDecoderBufferWriter create the data pipe and just pass the producer handle
to MojoDecoderBufferWriter.

If you want to share code for creating the data pipe, I would create a
standalone function (may be a static member function of MojoDecoderBufferWriter)
that accepts Audio/VideoDecoderConfig.

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/common/mo...
File media/mojo/common/mojo_decoder_buffer_converter.h (right):

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/common/mo...
media/mojo/common/mojo_decoder_buffer_converter.h:24: // Creates a
MojoDecoderBufferWriter of |type| and set the |producer_handle|.
is this a typo?

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/common/mo...
media/mojo/common/mojo_decoder_buffer_converter.h:37:
scoped_refptr<DecoderBuffer> ConvertDecoderBuffer(
A Convert function on a Reader seems a bit weird.

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/common/mo...
media/mojo/common/mojo_decoder_buffer_converter.h:52: static
std::unique_ptr<MojoDecoderBufferWriter> Create(
I would replace this with a CreateDataPipe function. It will be much more
clearer and flexible.

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/common/mo...
media/mojo/common/mojo_decoder_buffer_converter.h:64: mojom::DecoderBufferPtr
ConvertDecoderBuffer(
ditto. A Convert function on writer seems weird.

[xhwang, 2016-06-24 23:20:20]

comments addressed

[xhwang, 2016-06-24 23:44:12]

PTAL again

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/clients/m...
File media/mojo/clients/mojo_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/clients/m...
media/mojo/clients/mojo_audio_decoder.cc:166: mojo_decoder_buffer_writer_ =
MojoDecoderBufferWriter::Create(
On 2016/06/24 19:35:10, alokp wrote:
> To keep things clear and flexible I would suggest letting clients of
> MojoDecoderBufferWriter create the data pipe and just pass the producer handle
> to MojoDecoderBufferWriter.
> 
> If you want to share code for creating the data pipe, I would create a
> standalone function (may be a static member function of
MojoDecoderBufferWriter)
> that accepts Audio/VideoDecoderConfig.

That standalone function is pretty much the CreateDataPipe() function I have in
the converter.cc.

Yeah, I debated on this but couldn't find a perfect solution. This is the best I
can get so far.

I'd like to hide DataPipe details as much as possible. That's why I choose this
approach, so client of the writer doesn't need to deal with the producer handle
directly.

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/common/mo...
File media/mojo/common/mojo_decoder_buffer_converter.h (right):

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/common/mo...
media/mojo/common/mojo_decoder_buffer_converter.h:24: // Creates a
MojoDecoderBufferWriter of |type| and set the |producer_handle|.
On 2016/06/24 19:35:10, alokp wrote:
> is this a typo?

Done.

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/common/mo...
media/mojo/common/mojo_decoder_buffer_converter.h:37:
scoped_refptr<DecoderBuffer> ConvertDecoderBuffer(
On 2016/06/24 19:35:10, alokp wrote:
> A Convert function on a Reader seems a bit weird.

Done.

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/common/mo...
media/mojo/common/mojo_decoder_buffer_converter.h:52: static
std::unique_ptr<MojoDecoderBufferWriter> Create(
On 2016/06/24 19:35:10, alokp wrote:
> I would replace this with a CreateDataPipe function. It will be much more
> clearer and flexible.

See above.

https://chromiumcodereview.appspot.com/2096063003/diff/1/media/mojo/common/mo...
media/mojo/common/mojo_decoder_buffer_converter.h:64: mojom::DecoderBufferPtr
ConvertDecoderBuffer(
On 2016/06/24 19:35:10, alokp wrote:
> ditto. A Convert function on writer seems weird.

Done.

[xhwang, 2016-06-27 19:55:09]

kindly ping. I'd like to get this landed before I OOO. Thanks!

[alokp, 2016-06-27 20:18:22]

lgtm

mostly nits. looks very good.

https://codereview.chromium.org/2096063003/diff/20001/media/mojo/common/mojo_...
File media/mojo/common/mojo_decoder_buffer_converter.h (right):

https://codereview.chromium.org/2096063003/diff/20001/media/mojo/common/mojo_...
media/mojo/common/mojo_decoder_buffer_converter.h:5: #ifndef
MEDIA_MOJO_COMMON_MOJO_DECODER_BUFFER_CONVERTER_
nit: Since this file contains implementations of Reader and Writer, renaming it
to mojo_decoder_buffer_io.h may be more appropriate?

https://codereview.chromium.org/2096063003/diff/20001/media/mojo/common/mojo_...
media/mojo/common/mojo_decoder_buffer_converter.h:35: // Converts |buffer| into
a DecoderBuffer (read data from DataPipe if needed).
nit: Change "Converts" to "Reads decoder buffer data".

nit2: Perhaps rename this function to ReadDecoderBufferData to make it clearer.

Ditto for the writer.

[sandersd (OOO until July 31), 2016-06-27 20:24:55]

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/clien...
File media/mojo/clients/mojo_audio_decoder.cc (left):

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/clien...
media/mojo/clients/mojo_audio_decoder.cc:60: if
(remote_decoder_.encountered_error()) {
We use this pattern everywhere; what is the expected path now if there is an
error? (I'm assuming that it's not guaranteed that the error handler will run
after we return?)

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/commo...
File media/mojo/common/mojo_decoder_buffer_converter.cc (right):

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/commo...
media/mojo/common/mojo_decoder_buffer_converter.cc:34:
options.capacity_num_bytes = 1.5 * (1024 * 1024);
I have a preference for more headroom, since we can't (currently) recover from a
failure due to a too-small size.

(It's actually even worse than that: since we don't wait for space in Write, we
will try to push ~4 frames into this capacity and fail with a decode error if
that doesn't work.)

We need to increase this or solve the underlying issue (a resize mechanism).

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/commo...
media/mojo/common/mojo_decoder_buffer_converter.cc:37: // Choose a random size.
Nit: a hardcoded value is never random.

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/commo...
media/mojo/common/mojo_decoder_buffer_converter.cc:83: MOJO_DEADLINE_INDEFINITE,
&state);
VideoDecoderService comment was about this line.

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/servi...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/servi...
media/mojo/services/mojo_video_decoder_service.cc:95: // TODO(sandersd): Do not
wait indefinitely.
This TODO was for the implementation of reading buffers, not for
VideoDecoderService.

[sandersd (OOO until July 31), 2016-06-27 20:25:20]

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/clien...
File media/mojo/clients/mojo_audio_decoder.cc (left):

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/clien...
media/mojo/clients/mojo_audio_decoder.cc:60: if
(remote_decoder_.encountered_error()) {
On 2016/06/27 20:24:54, sandersd wrote:
> We use this pattern everywhere; what is the expected path now if there is an
> error? (I'm assuming that it's not guaranteed that the error handler will run
> after we return?)

*now guaranteed

[Ken Rockot(use gerrit already), 2016-06-27 21:00:26]

rockot@chromium.org changed reviewers:
+ rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-06-27 21:00:27]

https://codereview.chromium.org/2096063003/diff/20001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_audio_decoder.cc (left):

https://codereview.chromium.org/2096063003/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_audio_decoder.cc:60: if
(remote_decoder_.encountered_error()) {
On 2016/06/27 at 20:25:20, sandersd wrote:
> On 2016/06/27 20:24:54, sandersd wrote:
> > We use this pattern everywhere; what is the expected path now if there is an
> > error? (I'm assuming that it's not guaranteed that the error handler will
run
> > after we return?)
> 
> *now guaranteed

The correct thing to do is set an error handler and react to errors when they
happen.

[sandersd (OOO until July 31), 2016-06-27 21:04:21]

https://codereview.chromium.org/2096063003/diff/20001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_audio_decoder.cc (left):

https://codereview.chromium.org/2096063003/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_audio_decoder.cc:60: if
(remote_decoder_.encountered_error()) {
On 2016/06/27 21:00:27, Ken Rockot wrote:
> On 2016/06/27 at 20:25:20, sandersd wrote:
> > On 2016/06/27 20:24:54, sandersd wrote:
> > > We use this pattern everywhere; what is the expected path now if there is
an
> > > error? (I'm assuming that it's not guaranteed that the error handler will
> run
> > > after we return?)
> > 
> > *now guaranteed
> 
> The correct thing to do is set an error handler and react to errors when they
> happen.

Right, but what if an error has already happened? The implication is that this
isn't possible, which would be an improvement, but this code was written because
those guarantees were not available.

[Ken Rockot(use gerrit already), 2016-06-27 21:17:21]

https://codereview.chromium.org/2096063003/diff/20001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_audio_decoder.cc (left):

https://codereview.chromium.org/2096063003/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_audio_decoder.cc:60: if
(remote_decoder_.encountered_error()) {
On 2016/06/27 at 21:04:21, sandersd wrote:
> On 2016/06/27 21:00:27, Ken Rockot wrote:
> > On 2016/06/27 at 20:25:20, sandersd wrote:
> > > On 2016/06/27 20:24:54, sandersd wrote:
> > > > We use this pattern everywhere; what is the expected path now if there
is an
> > > > error? (I'm assuming that it's not guaranteed that the error handler
will
> > run
> > > > after we return?)
> > > 
> > > *now guaranteed
> > 
> > The correct thing to do is set an error handler and react to errors when
they
> > happen.
> 
> Right, but what if an error has already happened? The implication is that this
isn't possible, which would be an improvement, but this code was written because
those guarantees were not available.

encountered_error() only ever changes value in response to an error handler
being invoked. Error handlers are invoked asynchronously, and can't be invoked
until their subject (remote_decoder_ in this case) has been bound. As long as
you set an error handler immediately after binding you can't miss anything.

If there was a pipe error at the time of the Bind() call, it will post a task to
call the error handler.

[xhwang, 2016-06-28 01:08:38]

comments addressed

[xhwang, 2016-06-28 01:11:48]

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/clien...
File media/mojo/clients/mojo_audio_decoder.cc (left):

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/clien...
media/mojo/clients/mojo_audio_decoder.cc:60: if
(remote_decoder_.encountered_error()) {
On 2016/06/27 21:17:21, Ken Rockot wrote:
> On 2016/06/27 at 21:04:21, sandersd wrote:
> > On 2016/06/27 21:00:27, Ken Rockot wrote:
> > > On 2016/06/27 at 20:25:20, sandersd wrote:
> > > > On 2016/06/27 20:24:54, sandersd wrote:
> > > > > We use this pattern everywhere; what is the expected path now if there
> is an
> > > > > error? (I'm assuming that it's not guaranteed that the error handler
> will
> > > run
> > > > > after we return?)
> > > > 
> > > > *now guaranteed
> > > 
> > > The correct thing to do is set an error handler and react to errors when
> they
> > > happen.
> > 
> > Right, but what if an error has already happened? The implication is that
this
> isn't possible, which would be an improvement, but this code was written
because
> those guarantees were not available.
> 
> encountered_error() only ever changes value in response to an error handler
> being invoked. Error handlers are invoked asynchronously, and can't be invoked
> until their subject (remote_decoder_ in this case) has been bound. As long as
> you set an error handler immediately after binding you can't miss anything.
> 
> If there was a pipe error at the time of the Bind() call, it will post a task
to
> call the error handler.

What Ken said. Basically encountered_error() will always be true here before we
set the error handler. And the error handler will be called even if the error
happened *before* the error handler is set. The encountered_error() API is very
confusing, I think we should just stay away from it...

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/commo...
File media/mojo/common/mojo_decoder_buffer_converter.cc (right):

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/commo...
media/mojo/common/mojo_decoder_buffer_converter.cc:34:
options.capacity_num_bytes = 1.5 * (1024 * 1024);
On 2016/06/27 20:24:55, sandersd wrote:
> I have a preference for more headroom, since we can't (currently) recover from
a
> failure due to a too-small size.
> 
> (It's actually even worse than that: since we don't wait for space in Write,
we
> will try to push ~4 frames into this capacity and fail with a decode error if
> that doesn't work.)
> 
> We need to increase this or solve the underlying issue (a resize mechanism).

Updated to match what MojoVideoDecoder does today.

Agreed this could be improved. Maybe we could pass in a size in CreateDataPipe()
so that clients can customize this. The client knows about the
VideoDecoderConfig, and how many parallel decodes it supports, so it could have
a better estimate. Or as you suggested, it'd be nice to be able to adjust this
at run time (not sure how though).

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/commo...
media/mojo/common/mojo_decoder_buffer_converter.cc:37: // Choose a random size.
On 2016/06/27 20:24:55, sandersd wrote:
> Nit: a hardcoded value is never random.

Done.

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/commo...
media/mojo/common/mojo_decoder_buffer_converter.cc:83: MOJO_DEADLINE_INDEFINITE,
&state);
On 2016/06/27 20:24:54, sandersd wrote:
> VideoDecoderService comment was about this line.

Done.

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/commo...
File media/mojo/common/mojo_decoder_buffer_converter.h (right):

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/commo...
media/mojo/common/mojo_decoder_buffer_converter.h:5: #ifndef
MEDIA_MOJO_COMMON_MOJO_DECODER_BUFFER_CONVERTER_
On 2016/06/27 20:18:21, alokp wrote:
> nit: Since this file contains implementations of Reader and Writer, renaming
it
> to mojo_decoder_buffer_io.h may be more appropriate?

I still like converter better (the same reason why I use Convert* as function
name originally. These two classes not only read/write the data part, but also
does a full "conversion" between media::mojom::DecoderBuffer and
media::DecoderBuffer. It's very similar to the traditional mojo type converters,
so I'd like make the name consistent as well.

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/commo...
media/mojo/common/mojo_decoder_buffer_converter.h:35: // Converts |buffer| into
a DecoderBuffer (read data from DataPipe if needed).
On 2016/06/27 20:18:21, alokp wrote:
> nit: Change "Converts" to "Reads decoder buffer data".
> 
> nit2: Perhaps rename this function to ReadDecoderBufferData to make it
clearer.
> 
> Ditto for the writer.

See comment above. This function not only read the decoder buffer data, but also
do a full conversion (including other fields like timestamp etc).

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/servi...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://chromiumcodereview.appspot.com/2096063003/diff/20001/media/mojo/servi...
media/mojo/services/mojo_video_decoder_service.cc:95: // TODO(sandersd): Do not
wait indefinitely.
On 2016/06/27 20:24:55, sandersd wrote:
> This TODO was for the implementation of reading buffers, not for
> VideoDecoderService.

Done.

[xhwang, 2016-06-28 01:12:06]

PTAL again. Thanks!

[sandersd (OOO until July 31), 2016-06-28 01:18:13]

lgtm

[xhwang, 2016-06-28 05:36:05]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-06-28 05:36:06]

The patchset sent to the CQ was uploaded after l-g-t-m from alokp@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2096063003/#ps40001
(title: "comments addressed")

[commit-bot: I haz the power, 2016-06-28 05:36:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 06:39:08]

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

[commit-bot: I haz the power, 2016-06-28 06:39:09]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xhwang, 2016-06-28 16:47:52]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2016-06-28 16:48:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 17:18:47]

Description was changed from

==========
media: Add MojoDecoderBuffer{Reader|Writer}

This removes duplicate code in clients/services that transfer/receive
media DecoderBuffers over DataPipe.

The implementation of *::ConvertDecoderBuffer() is
copied from MojoAudioDecoder*, which has recently
been updated and reviewed by mojo owners
(https://chromiumcodereview.appspot.com/1982883003/)

BUG=621784
TEST=Moved unittest to a new file with improvements.
==========

to

==========
media: Add MojoDecoderBuffer{Reader|Writer}

This removes duplicate code in clients/services that transfer/receive
media DecoderBuffers over DataPipe.

The implementation of *::ConvertDecoderBuffer() is
copied from MojoAudioDecoder*, which has recently
been updated and reviewed by mojo owners
(https://chromiumcodereview.appspot.com/1982883003/)

BUG=621784
TEST=Moved unittest to a new file with improvements.
==========

[commit-bot: I haz the power, 2016-06-28 17:18:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-28 17:21:13]

Description was changed from

==========
media: Add MojoDecoderBuffer{Reader|Writer}

This removes duplicate code in clients/services that transfer/receive
media DecoderBuffers over DataPipe.

The implementation of *::ConvertDecoderBuffer() is
copied from MojoAudioDecoder*, which has recently
been updated and reviewed by mojo owners
(https://chromiumcodereview.appspot.com/1982883003/)

BUG=621784
TEST=Moved unittest to a new file with improvements.
==========

to

==========
media: Add MojoDecoderBuffer{Reader|Writer}

This removes duplicate code in clients/services that transfer/receive
media DecoderBuffers over DataPipe.

The implementation of *::ConvertDecoderBuffer() is
copied from MojoAudioDecoder*, which has recently
been updated and reviewed by mojo owners
(https://chromiumcodereview.appspot.com/1982883003/)

BUG=621784
TEST=Moved unittest to a new file with improvements.

Committed: https://crrev.com/1087d4ca6be621c1e687987b40df73c27783a800
Cr-Commit-Position: refs/heads/master@{#402496}
==========

[commit-bot: I haz the power, 2016-06-28 17:21:15]

Patchset 3 (id:??) landed as
https://crrev.com/1087d4ca6be621c1e687987b40df73c27783a800
Cr-Commit-Position: refs/heads/master@{#402496}