Implement disabling and enabling media tracks

media/blink/webmediaplayer_impl.cc

Index: media/blink/webmediaplayer_impl.cc
diff --git a/media/blink/webmediaplayer_impl.cc b/media/blink/webmediaplayer_impl.cc
index 3b373485a2ae011fd57fcf8bdd8e060aa1357bb7..bd58e7772fee1e6a975be1eed41e81ac82bd1948 100644
--- a/media/blink/webmediaplayer_impl.cc
+++ b/media/blink/webmediaplayer_impl.cc
@@ -577,12 +577,70 @@ bool WebMediaPlayerImpl::hasAudio() const {
   return pipeline_metadata_.has_audio;
 }
 
+using RestartStreamPlaybackCB =
+    base::Callback<void(DemuxerStream*, base::TimeDelta)>;
+
+void OnEnabledAudioTracksChanged(
+    Demuxer* demuxer,
+    base::TimeDelta currentTime,
+    const blink::WebVector<blink::WebMediaPlayer::TrackId>& enabledTrackIds,
+    const RestartStreamPlaybackCB& restart_stream_playback_cb) {
+  DemuxerStream* audio_stream = demuxer->GetStream(DemuxerStream::AUDIO);
+  bool enabled = false;
+  for (const auto& trackId : enabledTrackIds) {
+    const DemuxerStream* s = demuxer->GetDemuxerStreamByTrackId(trackId);
+    CHECK(s);
+    CHECK_EQ(s, audio_stream);
+    enabled = true;
+  }
+
+  DVLOG(1) << __FUNCTION__ << (enabled ? " enabling" : " disabling")
+           << " audio_stream=" << audio_stream
+           << " currentTime=" << currentTime.InSecondsF();
+  bool was_disabled = !audio_stream->enabled();
+  audio_stream->set_enabled(enabled, currentTime);
+  if (enabled && was_disabled) {
+    restart_stream_playback_cb.Run(audio_stream, currentTime);
+  }
+}
+
 void WebMediaPlayerImpl::enabledAudioTracksChanged(
     const blink::WebVector<blink::WebMediaPlayer::TrackId>& enabledTrackIds) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   CHECK(demuxer_.get());
 
-  // TODO(servolk): Implement disabling/enabling audio streams.
+  media_task_runner_->PostTask(

[chcunningham, 2016/06/02 23:49:42]

This is bit bouncy. 

Main Thread:   JS sets enabled on stream
Media Thread: calls set_enabled on demuxer_stream
Main Thread:   Call pipeline_->RestartStreamPlayback
Media Thread: Pipeline calls renderer->RestartStreamPlayback

...

Could we instead push more of the work to the pipeline and make just one post
from main -> media thread?

What if Pipeline::RestartStreamPlayback() instead became
Pipeline::OnEnableTracksChanged()? Maybe the name could be better (since video
is onSelectedTrackChanged), but the idea is pass the type, selected/enabled
id(s), and let pipeline post the task to media thread to set the enabled states
on the appropriate demuxer streams and restart renderers if needed. 

Also, pipeline can call its own GetMediaTime once on the media thread which
should improve its accuracy a little. 

[servolk, 2016/06/03 02:53:15]

Yes, we could do what you suggested, but one significant downside that I can
foresee is that we would either need to introduce external/blink track ids into
media::Pipeline public interface, probably not a good idea, since IIUC
blink-specific stuff is limited to media/blink/.
Or we'd need to find an alternative way of somehow delivering the information
from WMPI to Pipeline about which tracks exactly got enabled or disabled. I
guess we could cheat a little and instead of actually checking the track ids
given to us from blink level, we could just assume that there's max 1 audio and
1 video track, and then we can just check if audio |enabledTrackIds| vector or
video |selectedTrackId| are empty or not and then ask media::Pipeline to enable
or disable audio or video track without explicitly giving its blink id to
media::Pipeline. This solution would work (and would make the other CL that
implements mapping between blink track ids and demuxer streams unnecessary). But
it would require significant rework if we ever want to support multiple audio or
video tracks. I'm not sure if we want to go down that route. WDYT?

[chcunningham, 2016/06/03 20:54:59]

Let me see how our discussion here shakes out

https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...

Perhaps we'll just use bytstream track ids everywhere, which would address your
concern with this proposal. 

[xhwang, 2016/06/04 00:09:43]

For the thread hopping chcunningham pointed out, we don't really call demuxer
directly in WMPI, nor do we deal with the media_task_runner_ directly (the only
exception is SetSinkIdOnMediaThread() which I need to take another look). It
seems to me we should notify the Demuxer through PipelineController, similar to
how we do Seek().

Then you can probably just call something like demuxer->set_enabled(track_id,
enabled). (I am proposing to get rid of GetDemuxerStreamByTrackId(), see below.)

Then it would be nice if we could avoid the round trip of
OnSelectedVideoTrackChanged -> GetDemuxerStreamByTrackId ->
restart_stream_playback_cb -> RestartStreamPlayback. 

Thinking wildly, I wonder whether this can be solved mostly in the demuxer. For
example, we could have some like:

DemuxerStream::SetEnableNotification(cb);

Then in VideoRendererImpl:
demuxer_stream_->SetEnableDisableNotification(&OnDemuxerStreamEnabled);

void OnDemuxerStreamEnabled() {
  StartResumePlayback();
}

Then you don't have to go through the pipeline and renderer to resume playback.
When you enable the demuxer stream/track, the corresponding renderer will be
notified.

I haven't thought about all the details. Just something for us to think about...

[servolk, 2016/06/09 21:00:13]

There is a lot of different points here, let me try to address them in order
(can't find an easy way to quote here):

1. Yes, I guess we could go through pipeline controller instead of calling
demuxer directly in WMPI, that should be a relatively minor change, I'll give it
a try.

2. I'm not sure getting rid of GetDemuxerStreamByTrackId is a good idea (or
perhaps I just don't fully understand what you are suggesting instead). We get
media stack status notifications in blink as a set of media track ids. We need
to translate those ids into demuxerstreams somewhere. So I see two possible
options:
a. Pass the track ids sets into demuxer and make it do the translations
internally - is that what you are suggesting? I.e. we could add
Demuxer::OnEnabledAudioStreamChanged(<set of ids>) /
OnSelectedVideoStreamChanged(<id>) ? But that would mean we still introduce two
new functions into the public Demuxer interface. Those functions would operate
with track ids, but would not allow you to get access to demuxer streams by
knowing the track ids, which would make the usefulness of those functions very
limited (you wouldn't be able to do anything else with media track ids, only
enable/disable them).
b. (what's currently implemented in this CL) We could have a generic function in
the demuxer that would allow you to translate media track ids into demuxer
streams. Then you could do whatever you choose with those streams, you could
disable/enable them + you could also figure out other demuxer stream/media track
properties by knowing just the media track id (track type, liveness, a/v decoder
configs, etc). I think that would be much more useful function to have in the
public demuxer API.

3. Re your last point. We cannot avoid going at least through RendererImpl,
because we need to notify RendererImpl that the stream playback is being
restarted and that stream should be no longer considered 'ended'. That's why I
chose to implement it this way - it's already easy to recover A/VRendererImpls
from the 'ended' state by using their public APIs by just calling Flush +
StartPlaying(From), no further changes are necessary to the A/V renderers. But
there is currently no direct way to recover the media::RendererImpl from playing
just one stream (while the other one has been disabled/ended) back to playing
two streams.
And since we need to make some changes to the media::RendererImpl in any case,
and media::Renderer is already accessible from media::Pipeline(Controller), we
could propage the necessary changes through the pipeline(controller).
I guess we could make it slightly differently, along the lines of what you
described. I.e. introduce DemuxerStream::OnEnabledNotification, then in the
RendererImpl subscribe to those notifications through
RendererImpl::demuxer_stream_provider_, and when the media track status changes
we could enable/disable individual streams in the pipeline(controller) without
directly notifying media::Renderer. But I'm not sure how that would be better
than the current implementation.

+      FROM_HERE,
+      base::Bind(
+          &OnEnabledAudioTracksChanged, demuxer_.get(),
+          base::TimeDelta::FromSecondsD(currentTime()), enabledTrackIds,
+          BindToCurrentLoop(base::Bind(
+              &WebMediaPlayerImpl::RestartStreamPlayback, AsWeakPtr()))));
+}
+
+void OnSelectedVideoTrackChanged(
+    Demuxer* demuxer,
+    base::TimeDelta currentTime,
+    const std::vector<blink::WebMediaPlayer::TrackId>& selectedTrackIdVector,
+    const RestartStreamPlaybackCB& restart_stream_playback_cb) {
+  DemuxerStream* video_stream = demuxer->GetStream(DemuxerStream::VIDEO);
+  bool enabled = false;
+  if (!selectedTrackIdVector.empty()) {
+    const DemuxerStream* selectedVideoStream =
+        demuxer->GetDemuxerStreamByTrackId(selectedTrackIdVector[0]);
+    CHECK(selectedVideoStream);
+    CHECK_EQ(selectedVideoStream, video_stream);
+    enabled = true;
+  }
+
+  DVLOG(1) << __FUNCTION__ << (enabled ? " enabling" : " disabling")
+           << " video_stream=" << video_stream
+           << " currentTime=" << currentTime.InSecondsF();
+  bool was_disabled = !video_stream->enabled();
+  video_stream->set_enabled(enabled, currentTime);
+  if (enabled && was_disabled) {
+    restart_stream_playback_cb.Run(video_stream, currentTime);
+  }
 }
 
 void WebMediaPlayerImpl::selectedVideoTrackChanged(
@@ -590,7 +648,23 @@ void WebMediaPlayerImpl::selectedVideoTrackChanged(
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   CHECK(demuxer_.get());
 
-  // TODO(servolk): Implement disabling/enabling video stream.
+  std::vector<blink::WebMediaPlayer::TrackId> selectedTrackIdVector;
+  if (selectedTrackId) {
+    selectedTrackIdVector.push_back(*selectedTrackId);
+  }
+  media_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(
+          &OnSelectedVideoTrackChanged, demuxer_.get(),
+          base::TimeDelta::FromSecondsD(currentTime()), selectedTrackIdVector,
+          BindToCurrentLoop(base::Bind(
+              &WebMediaPlayerImpl::RestartStreamPlayback, AsWeakPtr()))));
+}
+
+void WebMediaPlayerImpl::RestartStreamPlayback(DemuxerStream* stream,
+                                               base::TimeDelta time) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  pipeline_.RestartStreamPlayback(stream, time);
 }
 
 blink::WebSize WebMediaPlayerImpl::naturalSize() const {