Support multiple media tracks in MSE / ChunkDemuxer

media/filters/chunk_demuxer.cc

Index: media/filters/chunk_demuxer.cc
diff --git a/media/filters/chunk_demuxer.cc b/media/filters/chunk_demuxer.cc
index 443c7c5558873ff0326294e29c72a53bb6ca0ab3..96b152c5ce8ed1bb95b508908476c82a18846735 100644
--- a/media/filters/chunk_demuxer.cc
+++ b/media/filters/chunk_demuxer.cc
@@ -6,7 +6,7 @@
 
 #include <algorithm>
 #include <limits>
-#include <list>
+#include <set>

[wolenetz, 2016/09/06 21:20:50]

nit: why did this change?

[servolk, 2016/09/07 01:35:10]

Because I don't see std::list being used explicitly in this file anywhere and
I'm now using std::set in OnEnabledAudioTracksChanged.

[wolenetz, 2016/09/12 21:41:05]

Acknowledged.

 #include <utility>
 
 #include "base/bind.h"
@@ -19,6 +19,7 @@
 #include "media/base/audio_decoder_config.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/media_tracks.h"
+#include "media/base/mime_util.h"
 #include "media/base/stream_parser_buffer.h"
 #include "media/base/timestamp_constants.h"
 #include "media/base/video_codecs.h"
@@ -491,11 +492,16 @@ base::Time ChunkDemuxer::GetTimelineOffset() const {
 DemuxerStream* ChunkDemuxer::GetStream(DemuxerStream::Type type) {
   DCHECK_NE(type, DemuxerStream::TEXT);
   base::AutoLock auto_lock(lock_);
-  if (type == DemuxerStream::VIDEO)
-    return video_.get();
 
   if (type == DemuxerStream::AUDIO)
-    return audio_.get();
+    for (const auto& s : audio_streams_)
+      if (s->enabled())
+        return s.get();
+
+  if (type == DemuxerStream::VIDEO)
+    for (const auto& s : video_streams_)
+      if (s->enabled())
+        return s.get();
 
   return NULL;
 }
@@ -506,8 +512,12 @@ TimeDelta ChunkDemuxer::GetStartTime() const {
 
 int64_t ChunkDemuxer::GetMemoryUsage() const {
   base::AutoLock auto_lock(lock_);
-  return (audio_ ? audio_->GetBufferedSize() : 0) +
-         (video_ ? video_->GetBufferedSize() : 0);
+  int64_t mem = 0;
+  for (const auto& s : audio_streams_)
+    mem += s->GetBufferedSize();
+  for (const auto& s : video_streams_)
+    mem += s->GetBufferedSize();
+  return mem;
 }
 
 void ChunkDemuxer::AbortPendingReads() {
@@ -561,31 +571,23 @@ void ChunkDemuxer::CancelPendingSeek(TimeDelta seek_time) {
 
 ChunkDemuxer::Status ChunkDemuxer::AddId(const std::string& id,
                                          const std::string& type,
-                                         std::vector<std::string>& codecs) {
+                                         const std::string& codecs) {
+  DVLOG(1) << __func__ << " id=" << id << " mime_type=" << type
+           << " codecs=" << codecs;
   base::AutoLock auto_lock(lock_);
 
   if ((state_ != WAITING_FOR_INIT && state_ != INITIALIZING) || IsValidId(id))
     return kReachedIdLimit;
 
-  bool has_audio = false;
-  bool has_video = false;
+  std::vector<std::string> parsed_codec_ids;
+  media::ParseCodecString(codecs, &parsed_codec_ids, false);
+
   std::unique_ptr<media::StreamParser> stream_parser(
-      StreamParserFactory::Create(type, codecs, media_log_, &has_audio,
-                                  &has_video));
+      StreamParserFactory::Create(type, parsed_codec_ids, media_log_));
 
   if (!stream_parser)
     return ChunkDemuxer::kNotSupported;
 
-  if ((has_audio && !source_id_audio_.empty()) ||
-      (has_video && !source_id_video_.empty()))
-    return kReachedIdLimit;
-
-  if (has_audio)
-    source_id_audio_ = id;
-
-  if (has_video)
-    source_id_video_ = id;
-
   std::unique_ptr<FrameProcessor> frame_processor(
       new FrameProcessor(base::Bind(&ChunkDemuxer::IncreaseDurationIfNecessary,
                                     base::Unretained(this)),
@@ -605,9 +607,15 @@ ChunkDemuxer::Status ChunkDemuxer::AddId(const std::string& id,
 
   pending_source_init_done_count_++;

[wolenetz, 2016/09/06 21:20:50]

Hmm. I think we may have some exacerbation of previous bad behavior which can
occur if:
addId(x);
removeId(x);
addId(y);
appendInitSegment(y);-->OnSourceInitDone().
At this point, we have just one SourceBuffer, and should be expecting only 1
init done, but a OnSourceInitDone() for (x) will never arrive and playback won't
reach HAVE_METADATA.

Furthermore, now we allow addId(z) *after* reaching HAVE_METADATA. I think
OnSourceInitDone() won't like this either.

[servolk, 2016/09/07 01:35:10]

Well, it's not exacerbation, it's actually exactly the same behavior that we
have now, only without the restriction of having only 1 audio and 1 video :)
I agree that it's a problem, but I believe it should be easy to fix - we can
remove the pending_source_init_done_count_ counter and instead just maintain a
map<id, bool> indicating whether a given id has been initialized or not. We'll
then check that map in the OnSourceInitDone and will proceed with the transition
into the initialized state only if all the ids have been initialized.

Re your second point, tbh I don't understand why you say '... now we allow ...',
could you explain? 
AFAICT I've preserved the existing logic: PipelineImpl reaches the HAVE_METADATA
state only after ChunkDemuxer invokes init_cb_, which happens from
ChunkDemuxer::OnSourceInitDone when all SourceBuffers/ids have received the init
segment. But that also means that ChunkDemuxer has transitioned from the
INITIALIZING to INITIALIZED state, and so we want allow any more SourceBuffers
added after that due to this check:
https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14731...

[wolenetz, 2016/09/12 21:41:05]

First point and map: SGTM. I'll review that.
Second point (was invalid point): Ack.

 
+  std::string expected_mss_codecs = codecs;

[wolenetz, 2016/09/06 21:20:49]

The parsing and expectation matching is confusing now that it's split even
further. Can this be more centralized?

[servolk, 2016/09/07 01:35:10]

Well, we can only do the matching in the OnNewConfigs where we get the
information about the actual codecs present in the bytestream. So I'm not sure
where else we can centralize it - any suggestions? Also the fact that video and
audio codecs use separate incompatible enums (AudioCodec and VideoCodec enums
respectively) makes it somewhat cumbersome to extract the codec matching logic
that is common between video and audio. I guess we could use a C++ template
function which would take enum type as a parameter, WDYT?

[wolenetz, 2016/09/12 21:41:05]

Would using the previous logic and something like (expect_audio) and
(expect_video) to enforce that the OnNewConfigs() contains tracks with at least
the expected audio and video tracks work? Or are you proposing deeper codec
parameter (profile, etc for avc, for example) be done in OnNewConfigs? I suppose
the route you're taking here might help if we want to differentiate, for
instance, h264 vs vp9 in ISO-BMFF at the OnNewConfigs() level. Or is there some
other reason I've missed why this logic is decentralized a bit further in this
CL?

+  if (codecs == "" && type == "audio/aac")
+    expected_mss_codecs = "aac";
+  if (codecs == "" && (type == "audio/mpeg" || type == "audio/mp3"))
+    expected_mss_codecs = "mp3";
+
   source_state->Init(
       base::Bind(&ChunkDemuxer::OnSourceInitDone, base::Unretained(this)),
-      has_audio, has_video, encrypted_media_init_data_cb_, new_text_track_cb);
+      expected_mss_codecs, encrypted_media_init_data_cb_, new_text_track_cb);
 
   source_state_map_[id] = source_state.release();
   return kOk;
@@ -627,12 +635,6 @@ void ChunkDemuxer::RemoveId(const std::string& id) {
 
   delete source_state_map_[id];
   source_state_map_.erase(id);
-
-  if (source_id_audio_ == id)
-    source_id_audio_.clear();
-
-  if (source_id_video_ == id)
-    source_id_video_.clear();
 }
 
 Ranges<TimeDelta> ChunkDemuxer::GetBufferedRanges(const std::string& id) const {
@@ -659,43 +661,55 @@ base::TimeDelta ChunkDemuxer::GetHighestPresentationTimestamp(
 void ChunkDemuxer::OnEnabledAudioTracksChanged(
     const std::vector<MediaTrack::Id>& track_ids,
     base::TimeDelta currTime) {
-  // Note: We intentionally don't lock here, since we are not accessing any
-  // members directly.
-  DemuxerStream* audio_stream = GetStream(DemuxerStream::AUDIO);
-  bool enabled = false;
-  CHECK(audio_stream);
-  DCHECK_LE(track_ids.size(), 1u);
-  if (track_ids.size() > 0) {
-#if DCHECK_IS_ON()
-    base::AutoLock auto_lock(lock_);
-    DCHECK(track_id_to_demux_stream_map_[track_ids[0]] == audio_stream);
-#endif
-    enabled = true;
+  base::AutoLock auto_lock(lock_);
+  std::set<DemuxerStream*> enabled_streams;
+  for (const auto& id : track_ids) {
+    DemuxerStream* stream = track_id_to_demux_stream_map_[id];
+    DCHECK(stream);
+    DCHECK_EQ(DemuxerStream::AUDIO, stream->type());
+    enabled_streams.insert(stream);
+  }
+
+  // First disable all streams that need to be disabled and then enable streams
+  // that are enabled.
+  for (const auto& stream : audio_streams_) {
+    if (enabled_streams.find(stream.get()) == enabled_streams.end()) {
+      DVLOG(1) << __func__ << ": disabling stream " << stream.get();
+      stream->set_enabled(false, currTime);
+    }
+  }
+  for (const auto& stream : enabled_streams) {
+    DVLOG(1) << __func__ << ": enabling stream " << stream;
+    stream->set_enabled(true, currTime);
   }
-  DVLOG(1) << __func__ << ": " << (enabled ? "enabling" : "disabling")
-           << " audio stream";
-  audio_stream->set_enabled(enabled, currTime);
 }
 
 void ChunkDemuxer::OnSelectedVideoTrackChanged(
     const std::vector<MediaTrack::Id>& track_ids,
     base::TimeDelta currTime) {
-  // Note: We intentionally don't lock here, since we are not accessing any
-  // members directly.
-  DemuxerStream* video_stream = GetStream(DemuxerStream::VIDEO);
-  bool enabled = false;
-  CHECK(video_stream);
   DCHECK_LE(track_ids.size(), 1u);
-  if (track_ids.size() > 0) {
-#if DCHECK_IS_ON()
-    base::AutoLock auto_lock(lock_);
-    DCHECK(track_id_to_demux_stream_map_[track_ids[0]] == video_stream);
-#endif
-    enabled = true;
+
+  base::AutoLock auto_lock(lock_);
+  DemuxerStream* selected_stream = nullptr;
+  if (!track_ids.empty()) {
+    selected_stream = track_id_to_demux_stream_map_[track_ids[0]];
+    DCHECK(selected_stream);
+    DCHECK_EQ(DemuxerStream::VIDEO, selected_stream->type());
+  }
+
+  // First disable all streams that need to be disabled and then enable the
+  // stream that needs to be enabled (if any).
+  for (const auto& stream : video_streams_) {
+    if (stream->type() == DemuxerStream::VIDEO &&
+        stream.get() != selected_stream) {
+      DVLOG(1) << __func__ << ": disabling stream " << stream.get();
+      stream->set_enabled(false, currTime);
+    }
+  }
+  if (selected_stream) {
+    DVLOG(1) << __func__ << ": enabling stream " << selected_stream;
+    selected_stream->set_enabled(true, currTime);
   }
-  DVLOG(1) << __func__ << ": " << (enabled ? "enabling" : "disabling")
-           << " video stream";
-  video_stream->set_enabled(enabled, currTime);
 }
 
 bool ChunkDemuxer::EvictCodedFrames(const std::string& id,
@@ -1049,7 +1063,7 @@ void ChunkDemuxer::OnSourceInitDone(
   DVLOG(1) << "OnSourceInitDone(" << params.duration.InSecondsF() << ")";
   lock_.AssertAcquired();
   DCHECK_EQ(state_, INITIALIZING);
-  if (!audio_ && !video_) {
+  if (audio_streams_.empty() && video_streams_.empty()) {
     ReportError_Locked(DEMUXER_ERROR_COULD_NOT_OPEN);
     return;
   }
@@ -1070,10 +1084,10 @@ void ChunkDemuxer::OnSourceInitDone(
   }
 
   if (params.liveness != DemuxerStream::LIVENESS_UNKNOWN) {

[wolenetz, 2016/09/06 21:20:50]

Hmm. We appear to be exacerbating previously confusing behavior here, where
depending on the order in which the OnSourceInitDone()'s occur, the liveness may
change both before *and* after reaching HAVE_METADATA. This may have unintended
consequences. Can this be cleaned up?

[servolk, 2016/09/07 01:35:10]

I believe this shouldn't be a problem, as HAVE_METADATA should only be reached
after OnSourceInitDone. See my comments above.

[wolenetz, 2016/09/12 21:41:05]

Ack, though IIUC, this code now allows a video track from one SourceBuffer to be
low_latency, and another video track from a distinct SourceBuffer to not be
low_latency. Does our track switching logic in the renderer allow for switching
among these? If so, not a problem here. 

-    if (audio_)
-      audio_->SetLiveness(params.liveness);
-    if (video_)
-      video_->SetLiveness(params.liveness);
+    for (const auto& s : audio_streams_)
+      s->SetLiveness(params.liveness);
+    for (const auto& s : video_streams_)
+      s->SetLiveness(params.liveness);
   }
 
   detected_audio_track_count_ += params.detected_audio_track_count;
@@ -1087,8 +1101,6 @@ void ChunkDemuxer::OnSourceInitDone(
     return;
 
   DCHECK_EQ(0, pending_source_init_done_count_);
-  DCHECK((source_id_audio_.empty() == !audio_) &&
-         (source_id_video_.empty() == !video_));
 
   // Record detected track counts by type corresponding to an MSE playback.
   // Counts are split into 50 buckets, capped into [0,100] range.
@@ -1099,10 +1111,10 @@ void ChunkDemuxer::OnSourceInitDone(
   UMA_HISTOGRAM_COUNTS_100("Media.MSE.DetectedTrackCount.Text",
                            detected_text_track_count_);
 
-  if (video_) {
+  for (const auto& s : video_streams_) {
     media_log_->RecordRapporWithSecurityOrigin(
         "Media.OriginUrl.MSE.VideoCodec." +
-        GetCodecName(video_->video_decoder_config().codec()));
+        GetCodecName(s->video_decoder_config().codec()));
   }
 
   SeekAllSources(GetStartTime());
@@ -1132,26 +1144,24 @@ ChunkDemuxerStream* ChunkDemuxer::CreateDemuxerStream(
   MediaTrack::Id media_track_id = GenerateMediaTrackId();
 
   switch (type) {
-    case DemuxerStream::AUDIO:
-      if (audio_)
-        return NULL;
-      audio_.reset(new ChunkDemuxerStream(
+    case DemuxerStream::AUDIO: {
+      std::unique_ptr<ChunkDemuxerStream> audio_stream(new ChunkDemuxerStream(
           DemuxerStream::AUDIO, splice_frames_enabled_, media_track_id));
       DCHECK(track_id_to_demux_stream_map_.find(media_track_id) ==
              track_id_to_demux_stream_map_.end());
-      track_id_to_demux_stream_map_[media_track_id] = audio_.get();
-      return audio_.get();
-      break;
-    case DemuxerStream::VIDEO:
-      if (video_)
-        return NULL;
-      video_.reset(new ChunkDemuxerStream(
+      track_id_to_demux_stream_map_[media_track_id] = audio_stream.get();
+      audio_streams_.push_back(std::move(audio_stream));
+      return audio_streams_.back().get();
+    } break;

[wolenetz, 2016/09/06 21:20:49]

inconsistent {...}break; vs {... break;}

[servolk, 2016/09/07 01:35:10]

Actually I just noticed that the break is redundant here, since there is a
return above it. Removed.

[wolenetz, 2016/09/12 21:41:05]

Acknowledged.

+    case DemuxerStream::VIDEO: {
+      std::unique_ptr<ChunkDemuxerStream> video_stream(new ChunkDemuxerStream(
           DemuxerStream::VIDEO, splice_frames_enabled_, media_track_id));
       DCHECK(track_id_to_demux_stream_map_.find(media_track_id) ==
              track_id_to_demux_stream_map_.end());
-      track_id_to_demux_stream_map_[media_track_id] = video_.get();
-      return video_.get();
-      break;
+      track_id_to_demux_stream_map_[media_track_id] = video_stream.get();
+      video_streams_.push_back(std::move(video_stream));
+      return video_streams_.back().get();
+    } break;

[wolenetz, 2016/09/06 21:20:50]

ditto

[servolk, 2016/09/07 01:35:10]

Done.

[wolenetz, 2016/09/12 21:41:05]

Acknowledged.

     case DemuxerStream::TEXT: {
       return new ChunkDemuxerStream(DemuxerStream::TEXT, splice_frames_enabled_,
                                     media_track_id);