media: Use associated interface for mojo AudioDecoderClient

media: Use associated interface for mojo AudioDecoderClient

Currently the AudioDecoderClient interface is running on a separate
message pipe than the main AudioDecoder interface. Hence the order of
AudioDecoder callbacks and AudioDecoderClient calls are not always
guaranteed. However, the media pipeline requires strict ordering of some
of these events. In this CL, we use associated interface for the client
so that client calls runs on the same message pipe as the main
interface, so that we can guarantee the delivery order.

BUG=646054
TEST=Verified by Cast team; unittests will be added right after this CL.

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

[xhwang, 2016-09-12 23:55:41]

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

[xhwang, 2016-09-12 23:55:42]

yzshen: This is my first time using associated interfaces, so I'd like you to
take a look to make sure I am using it correctly. I am working on adding some
unittests as well, but I don't know whether I can repro the out-of-order message
delivery in the first place. Is there any trick to trigger that?

tsunghung: FYI

[yzshen1, 2016-09-13 00:02:18]

On 2016/09/12 23:55:42, xhwang (slow) wrote:
> yzshen: This is my first time using associated interfaces, so I'd like you to
> take a look to make sure I am using it correctly. I am working on adding some
> unittests as well, but I don't know whether I can repro the out-of-order
message
> delivery in the first place.

(Not sure whether I have understood your question.)
Do you mean you want to repro out-of-order message delivery with the original,
non-associated interfaces?
We don't have a way to reliably trigger that. And there is no out-of-order
message delivery with associated interfaces (if the feature works correctly).


> Is there any trick to trigger that?
> 
> tsunghung: FYI

The change LGTM.

[xhwang, 2016-09-13 00:04:28]

On 2016/09/13 00:02:18, yzshen1 wrote:
> On 2016/09/12 23:55:42, xhwang (slow) wrote:
> > yzshen: This is my first time using associated interfaces, so I'd like you
to
> > take a look to make sure I am using it correctly. I am working on adding
some
> > unittests as well, but I don't know whether I can repro the out-of-order
> message
> > delivery in the first place.
> 
> (Not sure whether I have understood your question.)
> Do you mean you want to repro out-of-order message delivery with the original,
> non-associated interfaces?
> We don't have a way to reliably trigger that. And there is no out-of-order
> message delivery with associated interfaces (if the feature works correctly).

Yeah, I want to be able to repro the issue without this change, so I can verify
whether this change fixes the problem. Since there's no reliable way to trigger
that, I'll just see whether I can trigger it by multiple runs. Thank you for the
info! 
 
> > Is there any trick to trigger that?
> > 
> > tsunghung: FYI
> 
> The change LGTM.

[yzshen1, 2016-09-13 15:11:24]

On 2016/09/13 00:04:28, xhwang (slow) wrote:
> On 2016/09/13 00:02:18, yzshen1 wrote:
> > On 2016/09/12 23:55:42, xhwang (slow) wrote:
> > > yzshen: This is my first time using associated interfaces, so I'd like you
> to
> > > take a look to make sure I am using it correctly. I am working on adding
> some
> > > unittests as well, but I don't know whether I can repro the out-of-order
> > message
> > > delivery in the first place.
> > 
> > (Not sure whether I have understood your question.)
> > Do you mean you want to repro out-of-order message delivery with the
original,
> > non-associated interfaces?
> > We don't have a way to reliably trigger that. And there is no out-of-order
> > message delivery with associated interfaces (if the feature works
correctly).
> 
> Yeah, I want to be able to repro the issue without this change, so I can
verify
> whether this change fixes the problem. Since there's no reliable way to
trigger
> that, I'll just see whether I can trigger it by multiple runs.

Yeah. That is also the only way I can think up. :)

> Thank you for the
> info! 
>  
> > > Is there any trick to trigger that?
> > > 
> > > tsunghung: FYI
> > 
> > The change LGTM.

[AndyWu, 2016-09-13 17:30:07]

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

[AndyWu, 2016-09-13 17:30:07]

Usually, I can reproduce this issue with around 20~30 seek operations. With this
patch, I have tested more than 50 seek operations and could not reproduce the
issue.

[xhwang, 2016-09-13 17:38:32]

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

[xhwang, 2016-09-13 17:38:32]

dcheng: Could you please security review the one word change in
media/mojo/interfaces/audio_decoder.mojom? I am basically making the client
interface an associate interface.

[xhwang, 2016-09-13 17:39:08]

Description was changed from

==========
media: Use associated interface for mojo AudioDecoderClient

Currently the AudioDecoderClient interface is running on a separate
message pipe than the main AudioDecoder interface. Hence the order of
AudioDecoder callbacks and AudioDecoderClient calls are not always
guaranteed. However, the media pipeline requires strict ordering of some
of these events. In this CL, we use associated interface for the client
so that client calls runs on the same message pipe as the main
interface, so that we can guarantee the delivery order.

BUG=646054
TEST=To be verified by Cast team; unittests will be added shortly.
==========

to

==========
media: Use associated interface for mojo AudioDecoderClient

Currently the AudioDecoderClient interface is running on a separate
message pipe than the main AudioDecoder interface. Hence the order of
AudioDecoder callbacks and AudioDecoderClient calls are not always
guaranteed. However, the media pipeline requires strict ordering of some
of these events. In this CL, we use associated interface for the client
so that client calls runs on the same message pipe as the main
interface, so that we can guarantee the delivery order.

BUG=646054
TEST=Verified by Cast team; unittests will be added right after this CL.
==========

[dcheng, 2016-09-13 18:00:01]

lgtm

[xhwang, 2016-09-13 18:24:00]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2016-09-13 18:24:57]

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-09-13 20:58:38]

Description was changed from

==========
media: Use associated interface for mojo AudioDecoderClient

Currently the AudioDecoderClient interface is running on a separate
message pipe than the main AudioDecoder interface. Hence the order of
AudioDecoder callbacks and AudioDecoderClient calls are not always
guaranteed. However, the media pipeline requires strict ordering of some
of these events. In this CL, we use associated interface for the client
so that client calls runs on the same message pipe as the main
interface, so that we can guarantee the delivery order.

BUG=646054
TEST=Verified by Cast team; unittests will be added right after this CL.
==========

to

==========
media: Use associated interface for mojo AudioDecoderClient

Currently the AudioDecoderClient interface is running on a separate
message pipe than the main AudioDecoder interface. Hence the order of
AudioDecoder callbacks and AudioDecoderClient calls are not always
guaranteed. However, the media pipeline requires strict ordering of some
of these events. In this CL, we use associated interface for the client
so that client calls runs on the same message pipe as the main
interface, so that we can guarantee the delivery order.

BUG=646054
TEST=Verified by Cast team; unittests will be added right after this CL.
==========

[commit-bot: I haz the power, 2016-09-13 20:58:39]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-13 21:05:28]

Description was changed from

==========
media: Use associated interface for mojo AudioDecoderClient

Currently the AudioDecoderClient interface is running on a separate
message pipe than the main AudioDecoder interface. Hence the order of
AudioDecoder callbacks and AudioDecoderClient calls are not always
guaranteed. However, the media pipeline requires strict ordering of some
of these events. In this CL, we use associated interface for the client
so that client calls runs on the same message pipe as the main
interface, so that we can guarantee the delivery order.

BUG=646054
TEST=Verified by Cast team; unittests will be added right after this CL.
==========

to

==========
media: Use associated interface for mojo AudioDecoderClient

Currently the AudioDecoderClient interface is running on a separate
message pipe than the main AudioDecoder interface. Hence the order of
AudioDecoder callbacks and AudioDecoderClient calls are not always
guaranteed. However, the media pipeline requires strict ordering of some
of these events. In this CL, we use associated interface for the client
so that client calls runs on the same message pipe as the main
interface, so that we can guarantee the delivery order.

BUG=646054
TEST=Verified by Cast team; unittests will be added right after this CL.

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

[commit-bot: I haz the power, 2016-09-13 21:05:31]

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

[xhwang, 2016-09-14 17:48:51]

On 2016/09/13 21:05:31, commit-bot: I haz the power wrote:
> Patchset 1 (id:??) landed as
> https://crrev.com/e4bef74d708e121a6b7959a1d18e30fc5e6765d5
> Cr-Commit-Position: refs/heads/master@{#418364}

For the record, I wrote some unittest that returns 10 AudioBuffers per decode,
and I can repro the original issue on Android. It's falky, but almost always
repros when I run the test 100 times. With this CL, I cannot repro at all.

Note that I cannot repro the issue on desktop Linux. This may be due to how the
message loop and mojo message dispatching are implemented on different
platforms.

I'll clean up my local test and upload a test CL.