Make mojo renderer capable of supporting multiple streams/tracks

media/mojo/clients/mojo_renderer.cc

Index: media/mojo/clients/mojo_renderer.cc
diff --git a/media/mojo/clients/mojo_renderer.cc b/media/mojo/clients/mojo_renderer.cc
index 119de6240f6467c83f12bf0784d31676373b120d..e8e779442073adab0a0229ea93d3fed7a7981db7 100644
--- a/media/mojo/clients/mojo_renderer.cc
+++ b/media/mojo/clients/mojo_renderer.cc
@@ -78,28 +78,31 @@ void MojoRenderer::InitializeRendererFromStreams(
   DemuxerStream* const video =
       demuxer_stream_provider_->GetStream(DemuxerStream::VIDEO);
 
-  mojom::DemuxerStreamPtr audio_stream;
+  std::vector<mojom::DemuxerStreamPtr> streams;
   if (audio) {
-    audio_stream_.reset(
+    mojom::DemuxerStreamPtr audio_stream;
+    streams_.emplace_back(
         new MojoDemuxerStreamImpl(audio, MakeRequest(&audio_stream)));
-    // Using base::Unretained(this) is safe because |this| owns
-    // |audio_stream_|, and the error handler can't be invoked once
-    // |audio_stream_| is destroyed.
-    audio_stream_->set_connection_error_handler(
+    MojoDemuxerStreamImpl* mojo_stream = streams_.back().get();
+    // Using base::Unretained(this) is safe because |this| owns |mojo_stream|,
+    // and the error handler can't be invoked once |mojo_stream| is destroyed.
+    mojo_stream->set_connection_error_handler(
         base::Bind(&MojoRenderer::OnDemuxerStreamConnectionError,
-                   base::Unretained(this), DemuxerStream::AUDIO));
+                   base::Unretained(this), mojo_stream));

[xhwang, 2017/01/31 00:15:21]

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.

[xhwang, 2017/01/31 00:15:21]

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.

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

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.

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

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.

+    streams.push_back(std::move(audio_stream));
   }
 
-  mojom::DemuxerStreamPtr video_stream;
   if (video) {
-    video_stream_.reset(
+    mojom::DemuxerStreamPtr video_stream;
+    streams_.emplace_back(
         new MojoDemuxerStreamImpl(video, MakeRequest(&video_stream)));
-    // Using base::Unretained(this) is safe because |this| owns
-    // |video_stream_|, and the error handler can't be invoked once
-    // |video_stream_| is destroyed.
-    video_stream_->set_connection_error_handler(
+    MojoDemuxerStreamImpl* mojo_stream = streams_.back().get();
+    // Using base::Unretained(this) is safe because |this| owns |mojo_stream|,
+    // and the error handler can't be invoked once |mojo_stream| is destroyed.
+    mojo_stream->set_connection_error_handler(
         base::Bind(&MojoRenderer::OnDemuxerStreamConnectionError,
-                   base::Unretained(this), DemuxerStream::VIDEO));
+                   base::Unretained(this), mojo_stream));
+    streams.push_back(std::move(video_stream));
   }
 
   BindRemoteRendererIfNeeded();
@@ -111,8 +114,8 @@ void MojoRenderer::InitializeRendererFromStreams(
   // |remote_renderer_|, and the callback won't be dispatched if
   // |remote_renderer_| is destroyed.
   remote_renderer_->Initialize(
-      std::move(client_ptr_info), std::move(audio_stream),
-      std::move(video_stream), base::nullopt, base::nullopt,
+      std::move(client_ptr_info), std::move(streams), base::nullopt,
+      base::nullopt,
       base::Bind(&MojoRenderer::OnInitialized, base::Unretained(this), client));
 }
 
@@ -130,9 +133,9 @@ void MojoRenderer::InitializeRendererFromUrl(media::RendererClient* client) {
   // Using base::Unretained(this) is safe because |this| owns
   // |remote_renderer_|, and the callback won't be dispatched if
   // |remote_renderer_| is destroyed.
+  std::vector<mojom::DemuxerStreamPtr> streams;
   remote_renderer_->Initialize(
-      std::move(client_ptr_info), mojom::DemuxerStreamPtr(),
-      mojom::DemuxerStreamPtr(), url_params.media_url,
+      std::move(client_ptr_info), std::move(streams), url_params.media_url,
       url_params.first_party_for_cookies,
       base::Bind(&MojoRenderer::OnInitialized, base::Unretained(this), client));
 }
@@ -315,17 +318,18 @@ void MojoRenderer::OnConnectionError() {
     client_->OnError(PIPELINE_ERROR_DECODE);
 }
 
-void MojoRenderer::OnDemuxerStreamConnectionError(DemuxerStream::Type type) {
-  DVLOG(1) << __func__ << ": " << type;
+void MojoRenderer::OnDemuxerStreamConnectionError(
+    MojoDemuxerStreamImpl* stream) {
+  DVLOG(1) << __func__ << ": stream=" << stream;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  if (type == DemuxerStream::AUDIO) {
-    audio_stream_.reset();
-  } else if (type == DemuxerStream::VIDEO) {
-    video_stream_.reset();
-  } else {
-    NOTREACHED() << "Unexpected demuxer stream type: " << type;
+  for (auto& s : streams_) {
+    if (s.get() == stream) {

[xhwang, 2017/01/31 00:15:21]

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.

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

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.

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

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.

[servolk, 2017/01/31 17:38:04]

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.

+      s.reset();
+      return;
+    }
   }
+  NOTREACHED() << "Unrecognized demuxer stream=" << stream;
 }
 
 void MojoRenderer::BindRemoteRendererIfNeeded() {