Make mojo renderer capable of supporting multiple streams/tracks

Make mojo renderer capable of supporting multiple streams/tracks

BUG=487288
Review-Url: https://codereview.chromium.org/2383663002
Cr-Commit-Position: refs/heads/master@{#447296}
Committed: https://chromium.googlesource.com/chromium/src/+/29a95dae5a20fc981148d196d6500c66c4858ae5

[servolk, 2016-09-30 01:35:00]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 01:35:19]

Dry run: 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-30 02:04:13]

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

[commit-bot: I haz the power, 2016-09-30 02:04:13]

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

[servolk, 2016-09-30 17:28:33]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 17:29:17]

Dry run: 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-30 18:55:27]

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

[commit-bot: I haz the power, 2016-09-30 18:55:28]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-09-30 20:40:15]

Description was changed from

==========
Make mojo renderer capable of supporting multiple streams/tracks

BUG=487288
==========

to

==========
Make mojo renderer capable of supporting multiple streams/tracks

BUG=487288
==========

[servolk, 2016-09-30 20:40:15]

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

[xhwang, 2016-09-30 23:06:26]

Thanks for the CL! I just replied to the offline email. Let's discuss there
first, then this CL should be trivial (or at least easier) to review.

[servolk, 2016-11-02 00:18:31]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 00:19:13]

Dry run: 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-11-02 01:50:50]

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

[commit-bot: I haz the power, 2016-11-02 01:50:51]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-11-10 02:38:16]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 02:38:38]

Dry run: 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-11-10 04:51:05]

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

[commit-bot: I haz the power, 2016-11-10 04:51:06]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-12-02 23:02:42]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 23:03:37]

Dry run: 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-12-02 23:07:05]

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

[commit-bot: I haz the power, 2016-12-02 23:07:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[servolk, 2016-12-02 23:25:11]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 23:25:51]

Dry run: 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-12-02 23:36:05]

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

[commit-bot: I haz the power, 2016-12-02 23:36:06]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[servolk, 2016-12-02 23:46:09]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 23:46:46]

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

[servolk, 2016-12-02 23:49:23]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 23:50:18]

Dry run: 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-12-03 01:37:15]

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

[commit-bot: I haz the power, 2016-12-03 01:37:16]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-12-07 03:21:06]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 03:21:29]

Dry run: 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-12-07 06:33:01]

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

[commit-bot: I haz the power, 2016-12-07 06:33:01]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-12-14 01:24:38]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 01:25:20]

Dry run: 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-12-14 03:05:18]

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

[commit-bot: I haz the power, 2016-12-14 03:05:19]

Dry run: This issue passed the CQ dry run.

[servolk, 2017-01-03 23:07:48]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 23:08:13]

Dry run: 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, 2017-01-04 01:43:38]

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

[commit-bot: I haz the power, 2017-01-04 01:43:38]

Dry run: This issue passed the CQ dry run.

[servolk, 2017-01-12 17:31:08]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 17:32:13]

Dry run: 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, 2017-01-12 20:26:58]

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

[commit-bot: I haz the power, 2017-01-12 20:27:00]

Dry run: This issue passed the CQ dry run.

[servolk, 2017-01-24 01:36:06]

On 2016/09/30 23:06:26, xhwang_slow wrote:
> Thanks for the CL! I just replied to the offline email. Let's discuss there
> first, then this CL should be trivial (or at least easier) to review.

Btw, Xiaohan, now that we've started the discussion about refactoring
DemuxerStreamProvider, do you want to take another look at this CL? It fixes a
few places in mojo where we used to make an assumption that there's only one
DemuxerStream of each type, and instead changes the code to be multi-track
capable.

[servolk, 2017-01-24 01:36:15]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 01:36:47]

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

[servolk, 2017-01-24 01:46:47]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 01:47:47]

Dry run: 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, 2017-01-24 03:33:08]

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

[commit-bot: I haz the power, 2017-01-24 03:33:09]

Dry run: This issue passed the CQ dry run.

[servolk, 2017-01-25 01:57:24]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 01:57:58]

Dry run: 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, 2017-01-25 03:20:56]

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

[commit-bot: I haz the power, 2017-01-25 03:20:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[servolk, 2017-01-25 23:56:39]

On 2017/01/24 01:36:06, servolk wrote:
> On 2016/09/30 23:06:26, xhwang_slow wrote:
> > Thanks for the CL! I just replied to the offline email. Let's discuss there
> > first, then this CL should be trivial (or at least easier) to review.
> 
> Btw, Xiaohan, now that we've started the discussion about refactoring
> DemuxerStreamProvider, do you want to take another look at this CL? It fixes a
> few places in mojo where we used to make an assumption that there's only one
> DemuxerStream of each type, and instead changes the code to be multi-track
> capable.

ping

[servolk, 2017-01-30 17:24:24]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 17:24:40]

Dry run: 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, 2017-01-30 19:16:29]

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

[commit-bot: I haz the power, 2017-01-30 19:16:30]

Dry run: This issue passed the CQ dry run.

[servolk, 2017-01-30 19:19:12]

On 2017/01/25 23:56:39, servolk wrote:
> On 2017/01/24 01:36:06, servolk wrote:
> > On 2016/09/30 23:06:26, xhwang_slow wrote:
> > > Thanks for the CL! I just replied to the offline email. Let's discuss
there
> > > first, then this CL should be trivial (or at least easier) to review.
> > 
> > Btw, Xiaohan, now that we've started the discussion about refactoring
> > DemuxerStreamProvider, do you want to take another look at this CL? It fixes
a
> > few places in mojo where we used to make an assumption that there's only one
> > DemuxerStream of each type, and instead changes the code to be multi-track
> > capable.
> 
> ping

ping

[xhwang, 2017-01-30 19:39:55]

Sorry again for the delay.

For the record, servolk and I chat offline about the DemuxerStreamProvider (DSP)
interface refactoring design. We agreed that:
1. It might be cleaner to just have one notification callback on DSP about
stream status changes, rather than having a callback on every stream.
2. It's okay to refactor DemuxerStreamProvider first, then investigate how to
support multiple audio streams in the Renderer.

With that, I thought this CL needs to wait until the the design doc and
https://chromiumcodereview.appspot.com/2491043003/ to be updated (correct me if
I am wrong). I just checked, it seems
https://chromiumcodereview.appspot.com/2491043003/ has already been updated (but
I didn't receive any message about the update). And the design is not updated
yet.

Could you please update the design doc with the new design? Meanwhile I'll take
a look at https://chromiumcodereview.appspot.com/2491043003/. After that, I'll
take a look at this CL. Does this order look good to you?

Also, I noticed that the StreamStatusChangeCB is not hooked up when using
MojoRenderer. What's the plan for that part?

[servolk, 2017-01-30 19:48:15]

On 2017/01/30 19:39:55, xhwang_slow wrote:
> Sorry again for the delay.
> 
> For the record, servolk and I chat offline about the DemuxerStreamProvider
(DSP)
> interface refactoring design. We agreed that:
> 1. It might be cleaner to just have one notification callback on DSP about
> stream status changes, rather than having a callback on every stream.
> 2. It's okay to refactor DemuxerStreamProvider first, then investigate how to
> support multiple audio streams in the Renderer.
> 
> With that, I thought this CL needs to wait until the the design doc and
> https://chromiumcodereview.appspot.com/2491043003/ to be updated (correct me
if
> I am wrong). I just checked, it seems
> https://chromiumcodereview.appspot.com/2491043003/ has already been updated
(but
> I didn't receive any message about the update). And the design is not updated
> yet.
> 
> Could you please update the design doc with the new design? Meanwhile I'll
take
> a look at https://chromiumcodereview.appspot.com/2491043003/. After that, I'll
> take a look at this CL. Does this order look good to you?
> 
> Also, I noticed that the StreamStatusChangeCB is not hooked up when using
> MojoRenderer. What's the plan for that part?

I have mentioned our discussion in the comment I've made in the
DemuxerStreamProvider refactoring proposal doc, but yes, I'll update the doc to
specifically say that notifications are being moved from individual streams to
the MediaResource.
I'm planning to support StreamStatusChangeCB in a separate CL, since the
https://chromiumcodereview.appspot.com/2491043003/ is already pretty large.
And btw, the https://chromiumcodereview.appspot.com/2491043003/ depends on this
CL, so it's probably better to review this one first, and then
https://chromiumcodereview.appspot.com/2491043003/, but I'll leave it up to you.

[xhwang, 2017-01-31 00:15:22]

looking mostly good. just a few nits/comments.

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer.cc (right):

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.cc:91: base::Unretained(this), mojo_stream));
For future discussion: I am not a fan of using pointer as ID. Wondering whether
we should introduce a concept of stream ID which we can use here, and in other
places, e.g. StreamStatusUpdteCB.

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.cc:91: base::Unretained(this), mojo_stream));
nit: It's a bit weird that we emplace_back the stream first, then set this
handler on the raw pointer.

Can we do this instead?
- create MojoDemuxerStreamImpl
- set_connection_error_handler
- emplace_back

If you don't use the ID as mentioned above, you'll still need the raw pointer
for the error handler callback, but that should be a separate issue.

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.cc:327: if (s.get() == stream) {
ditto about using pointer as ID for comparison... Let me know what you think. If
we want to do it, it can/should be in a separate CL.

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer.h (right):

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.h:100: void
OnDemuxerStreamConnectionError(MojoDemuxerStreamImpl* stream);
Add comment about what |stream| means in this API (e.g. the stream where the
error happened on..)

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.h:134:
std::vector<std::unique_ptr<MojoDemuxerStreamImpl>> streams_;
nit: include <vector> and <memory>

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/interfaces/...
File media/mojo/interfaces/renderer.mojom (right):

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/interfaces/...
media/mojo/interfaces/renderer.mojom:18: array<DemuxerStream> streams,
Should this be optional if we actually use URL based rendering? Or |stream| will
be empty in this case?

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/services/de...
File media/mojo/services/demuxer_stream_provider_shim.cc (right):

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/services/de...
media/mojo/services/demuxer_stream_provider_shim.cc:38: return stream.get();
Add a comment that we only return the first stream, and a TODO to support
multiple streams.

[servolk, 2017-01-31 01:10:40]

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer.cc (right):

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.cc:91: base::Unretained(this), mojo_stream));
On 2017/01/31 00:15:21, xhwang_slow wrote:
> For future discussion: I am not a fan of using pointer as ID. Wondering
whether
> we should introduce a concept of stream ID which we can use here, and in other
> places, e.g. StreamStatusUpdteCB.

I understand your concern in general, but TBH in this particular case I think
using the pointer is fine. We are not really using it as an ID, we are using it
to indicate which mojo_stream has got the error. Having the
MojoDemuxerStreamImpl* the error handler should be able to figure out easily the
stream ID (if there is one) or type, or whatever other info we deem useful to
report on failure.

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.cc:91: base::Unretained(this), mojo_stream));
On 2017/01/31 00:15:21, xhwang_slow wrote:
> nit: It's a bit weird that we emplace_back the stream first, then set this
> handler on the raw pointer.
> 
> Can we do this instead?
> - create MojoDemuxerStreamImpl
> - set_connection_error_handler
> - emplace_back
> 
> If you don't use the ID as mentioned above, you'll still need the raw pointer
> for the error handler callback, but that should be a separate issue.

Done. Although it's a tiny bit more inefficient, since we have to move the
std::unique_ptr<MojoDemuxerStreamImpl> after construction. Emplace_back allowed
us to create it in-place right in the vector. But it's probably not a big deal.

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.cc:327: if (s.get() == stream) {
On 2017/01/31 00:15:21, xhwang_slow wrote:
> ditto about using pointer as ID for comparison... Let me know what you think.
If
> we want to do it, it can/should be in a separate CL.

TBH I think that using an ID instead of pointer would only complicate things
here, with no benefits. If we wanted to include more information about the
failed stream in the log (e.g. the stream type, or info about stream config)
we'd have to translate ID into the stream pointer first anyway. And the
OnDemuxerStreamConnectionError is not a public interface, it's an
internal/private function, so why is it bad to use stream id? This class knows
about MojoDemuxerStreamImpl anyway.

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer.h (right):

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.h:100: void
OnDemuxerStreamConnectionError(MojoDemuxerStreamImpl* stream);
On 2017/01/31 00:15:21, xhwang_slow wrote:
> Add comment about what |stream| means in this API (e.g. the stream where the
> error happened on..)

Done.

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.h:134:
std::vector<std::unique_ptr<MojoDemuxerStreamImpl>> streams_;
On 2017/01/31 00:15:21, xhwang_slow wrote:
> nit: include <vector> and <memory>

Done.

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/interfaces/...
File media/mojo/interfaces/renderer.mojom (right):

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/interfaces/...
media/mojo/interfaces/renderer.mojom:18: array<DemuxerStream> streams,
On 2017/01/31 00:15:21, xhwang_slow wrote:
> Should this be optional if we actually use URL based rendering? Or |stream|
will
> be empty in this case?

Done (made the streams parameter optional, also updated the comment to explain
that it's either streams or media_url).

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/services/de...
File media/mojo/services/demuxer_stream_provider_shim.cc (right):

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/services/de...
media/mojo/services/demuxer_stream_provider_shim.cc:38: return stream.get();
On 2017/01/31 00:15:21, xhwang_slow wrote:
> Add a comment that we only return the first stream, and a TODO to support
> multiple streams.

Done. Although https://codereview.chromium.org/2491043003/ already has changes
which make this comment/TODO immediately obsolete :)

[servolk, 2017-01-31 01:10:41]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[servolk, 2017-01-31 01:10:55]

servolk@chromium.org changed reviewers:
+ kenrb@chromium.org

[servolk, 2017-01-31 01:11:12]

On 2017/01/31 01:10:40, servolk wrote:
>
https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
> File media/mojo/clients/mojo_renderer.cc (right):
> 
>
https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
> media/mojo/clients/mojo_renderer.cc:91: base::Unretained(this), mojo_stream));
> On 2017/01/31 00:15:21, xhwang_slow wrote:
> > For future discussion: I am not a fan of using pointer as ID. Wondering
> whether
> > we should introduce a concept of stream ID which we can use here, and in
other
> > places, e.g. StreamStatusUpdteCB.
> 
> I understand your concern in general, but TBH in this particular case I think
> using the pointer is fine. We are not really using it as an ID, we are using
it
> to indicate which mojo_stream has got the error. Having the
> MojoDemuxerStreamImpl* the error handler should be able to figure out easily
the
> stream ID (if there is one) or type, or whatever other info we deem useful to
> report on failure.
> 
>
https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
> media/mojo/clients/mojo_renderer.cc:91: base::Unretained(this), mojo_stream));
> On 2017/01/31 00:15:21, xhwang_slow wrote:
> > nit: It's a bit weird that we emplace_back the stream first, then set this
> > handler on the raw pointer.
> > 
> > Can we do this instead?
> > - create MojoDemuxerStreamImpl
> > - set_connection_error_handler
> > - emplace_back
> > 
> > If you don't use the ID as mentioned above, you'll still need the raw
pointer
> > for the error handler callback, but that should be a separate issue.
> 
> Done. Although it's a tiny bit more inefficient, since we have to move the
> std::unique_ptr<MojoDemuxerStreamImpl> after construction. Emplace_back
allowed
> us to create it in-place right in the vector. But it's probably not a big
deal.
> 
>
https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
> media/mojo/clients/mojo_renderer.cc:327: if (s.get() == stream) {
> On 2017/01/31 00:15:21, xhwang_slow wrote:
> > ditto about using pointer as ID for comparison... Let me know what you
think.
> If
> > we want to do it, it can/should be in a separate CL.
> 
> TBH I think that using an ID instead of pointer would only complicate things
> here, with no benefits. If we wanted to include more information about the
> failed stream in the log (e.g. the stream type, or info about stream config)
> we'd have to translate ID into the stream pointer first anyway. And the
> OnDemuxerStreamConnectionError is not a public interface, it's an
> internal/private function, so why is it bad to use stream id? This class knows
> about MojoDemuxerStreamImpl anyway.
> 
>
https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
> File media/mojo/clients/mojo_renderer.h (right):
> 
>
https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
> media/mojo/clients/mojo_renderer.h:100: void
> OnDemuxerStreamConnectionError(MojoDemuxerStreamImpl* stream);
> On 2017/01/31 00:15:21, xhwang_slow wrote:
> > Add comment about what |stream| means in this API (e.g. the stream where the
> > error happened on..)
> 
> Done.
> 
>
https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
> media/mojo/clients/mojo_renderer.h:134:
> std::vector<std::unique_ptr<MojoDemuxerStreamImpl>> streams_;
> On 2017/01/31 00:15:21, xhwang_slow wrote:
> > nit: include <vector> and <memory>
> 
> Done.
> 
>
https://codereview.chromium.org/2383663002/diff/280001/media/mojo/interfaces/...
> File media/mojo/interfaces/renderer.mojom (right):
> 
>
https://codereview.chromium.org/2383663002/diff/280001/media/mojo/interfaces/...
> media/mojo/interfaces/renderer.mojom:18: array<DemuxerStream> streams,
> On 2017/01/31 00:15:21, xhwang_slow wrote:
> > Should this be optional if we actually use URL based rendering? Or |stream|
> will
> > be empty in this case?
> 
> Done (made the streams parameter optional, also updated the comment to explain
> that it's either streams or media_url).
> 
>
https://codereview.chromium.org/2383663002/diff/280001/media/mojo/services/de...
> File media/mojo/services/demuxer_stream_provider_shim.cc (right):
> 
>
https://codereview.chromium.org/2383663002/diff/280001/media/mojo/services/de...
> media/mojo/services/demuxer_stream_provider_shim.cc:38: return stream.get();
> On 2017/01/31 00:15:21, xhwang_slow wrote:
> > Add a comment that we only return the first stream, and a TODO to support
> > multiple streams.
> 
> Done. Although https://codereview.chromium.org/2491043003/ already has changes
> which make this comment/TODO immediately obsolete :)

+kenrb@ for media/mojo/interfaces/renderer.mojom

[commit-bot: I haz the power, 2017-01-31 01:11:17]

Dry run: 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, 2017-01-31 03:02:33]

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

[commit-bot: I haz the power, 2017-01-31 03:02:34]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-01-31 07:49:07]

lgtm % nits

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer.cc (right):

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.cc:327: if (s.get() == stream) {
On 2017/01/31 01:10:40, servolk wrote:
> On 2017/01/31 00:15:21, xhwang_slow wrote:
> > ditto about using pointer as ID for comparison... Let me know what you
think.
> If
> > we want to do it, it can/should be in a separate CL.
> 
> TBH I think that using an ID instead of pointer would only complicate things
> here, with no benefits. If we wanted to include more information about the
> failed stream in the log (e.g. the stream type, or info about stream config)
> we'd have to translate ID into the stream pointer first anyway. And the
> OnDemuxerStreamConnectionError is not a public interface, it's an
> internal/private function, so why is it bad to use stream id? This class knows
> about MojoDemuxerStreamImpl anyway.

If we use ID, |streams_| will probably be a map instead of a vector. Getting the
stream by the ID would be trivial to do.

https://codereview.chromium.org/2383663002/diff/300001/media/mojo/services/de...
File media/mojo/services/demuxer_stream_provider_shim.cc (right):

https://codereview.chromium.org/2383663002/diff/300001/media/mojo/services/de...
media/mojo/services/demuxer_stream_provider_shim.cc:35: // TODO: Make this work
with multiple streams.
nit: TODO(ldap)

[servolk, 2017-01-31 17:37:54]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[servolk, 2017-01-31 17:38:09]

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
File media/mojo/clients/mojo_renderer.cc (right):

https://codereview.chromium.org/2383663002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_renderer.cc:327: if (s.get() == stream) {
On 2017/01/31 07:49:07, xhwang_slow wrote:
> On 2017/01/31 01:10:40, servolk wrote:
> > On 2017/01/31 00:15:21, xhwang_slow wrote:
> > > ditto about using pointer as ID for comparison... Let me know what you
> think.
> > If
> > > we want to do it, it can/should be in a separate CL.
> > 
> > TBH I think that using an ID instead of pointer would only complicate things
> > here, with no benefits. If we wanted to include more information about the
> > failed stream in the log (e.g. the stream type, or info about stream config)
> > we'd have to translate ID into the stream pointer first anyway. And the
> > OnDemuxerStreamConnectionError is not a public interface, it's an
> > internal/private function, so why is it bad to use stream id? This class
knows
> > about MojoDemuxerStreamImpl anyway.
> 
> If we use ID, |streams_| will probably be a map instead of a vector. Getting
the
> stream by the ID would be trivial to do.

Yes, I understand that we could trivially map the id to the actual pointer, but
my question was about why would we want to do this one extra mapping step, when
we can just pass in the pointer directly? If this function was externally
accessible I would agree that we need an ID. But this is internal function and
it doesn't expose the MojoDemuxerStreamImpl* pointer to any external code. We do
already have access to MojoDemuxerStreamImpl* pointers inside this class and we
are not going to expose them further (outside of this class) through this
change. So I think there's no advantage to introducing an ID in here.

https://codereview.chromium.org/2383663002/diff/300001/media/mojo/services/de...
File media/mojo/services/demuxer_stream_provider_shim.cc (right):

https://codereview.chromium.org/2383663002/diff/300001/media/mojo/services/de...
media/mojo/services/demuxer_stream_provider_shim.cc:35: // TODO: Make this work
with multiple streams.
On 2017/01/31 07:49:07, xhwang_slow wrote:
> nit: TODO(ldap)

Done.

[commit-bot: I haz the power, 2017-01-31 17:38:16]

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

[kenrb, 2017-01-31 18:43:54]

mojo lgtm

[servolk, 2017-01-31 18:44:37]

The CQ bit was unchecked by servolk@chromium.org

[servolk, 2017-01-31 18:44:58]

The CQ bit was checked by servolk@chromium.org

[servolk, 2017-01-31 18:44:58]

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/2383663002/#ps320001
(title: "nit")

[commit-bot: I haz the power, 2017-01-31 18:45:53]

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, 2017-01-31 19:33:53]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1485888298050450,
"parent_rev": "91f22543db00f2709dc644ffb509de6cd9839762", "commit_rev":
"29a95dae5a20fc981148d196d6500c66c4858ae5"}

[commit-bot: I haz the power, 2017-01-31 19:34:32]

Description was changed from

==========
Make mojo renderer capable of supporting multiple streams/tracks

BUG=487288
==========

to

==========
Make mojo renderer capable of supporting multiple streams/tracks

BUG=487288

Review-Url: https://codereview.chromium.org/2383663002
Cr-Commit-Position: refs/heads/master@{#447296}
Committed:
https://chromium.googlesource.com/chromium/src/+/29a95dae5a20fc981148d196d650...
==========

[commit-bot: I haz the power, 2017-01-31 19:34:38]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/29a95dae5a20fc981148d196d650...