Support multiple media tracks in MSE / ChunkDemuxer

media/filters/chunk_demuxer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/filters/chunk_demuxer.h"
 
 #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"
 #include "base/callback_helpers.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #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"
 #include "media/base/video_decoder_config.h"
 #include "media/filters/frame_processor.h"
 #include "media/filters/stream_parser_factory.h"
 
 using base::TimeDelta;
 
 namespace media {
@@ skipping 452 matching lines @@
 }
 
 // Demuxer implementation.
 base::Time ChunkDemuxer::GetTimelineOffset() const {
   return timeline_offset_;
 }
 
 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;
 }
 
 TimeDelta ChunkDemuxer::GetStartTime() const {
   return TimeDelta();
 }
 
 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() {
   base::AutoLock auto_lock(lock_);
   DCHECK(state_ == INITIALIZED || state_ == ENDED || state_ == SHUTDOWN ||
          state_ == PARSE_ERROR)
       << state_;
 
   if (state_ == SHUTDOWN || state_ == PARSE_ERROR)
     return;
@@ skipping 33 matching lines @@
   if (seek_cb_.is_null()) {
     cancel_next_seek_ = true;
     return;
   }
 
   base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK);
 }
 
 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)),
                          media_log_));
 
   std::unique_ptr<MediaSourceState> source_state(new MediaSourceState(
       std::move(stream_parser), std::move(frame_processor),
       base::Bind(&ChunkDemuxer::CreateDemuxerStream, base::Unretained(this)),
       media_log_));
 
   MediaSourceState::NewTextTrackCB new_text_track_cb;
 
   if (enable_text_) {
     new_text_track_cb = base::Bind(&ChunkDemuxer::OnNewTextTrack,
                                    base::Unretained(this));
   }
 
   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;
 }
 
 void ChunkDemuxer::SetTracksWatcher(
     const std::string& id,
     const MediaTracksUpdatedCB& tracks_updated_cb) {
   base::AutoLock auto_lock(lock_);
   CHECK(IsValidId(id));
   source_state_map_[id]->SetTracksWatcher(tracks_updated_cb);
 }
 
 void ChunkDemuxer::RemoveId(const std::string& id) {
   base::AutoLock auto_lock(lock_);
   CHECK(IsValidId(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 {
   base::AutoLock auto_lock(lock_);
   DCHECK(!id.empty());
 
   MediaSourceStateMap::const_iterator itr = source_state_map_.find(id);
 
   DCHECK(itr != source_state_map_.end());
   return itr->second->GetBufferedRanges(duration_, state_ == ENDED);
 }
 
 base::TimeDelta ChunkDemuxer::GetHighestPresentationTimestamp(
     const std::string& id) const {
   base::AutoLock auto_lock(lock_);
   DCHECK(!id.empty());
 
   MediaSourceStateMap::const_iterator itr = source_state_map_.find(id);
 
   DCHECK(itr != source_state_map_.end());
   return itr->second->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);
   }
-  DVLOG(1) << __func__ << ": " << (enabled ? "enabling" : "disabling")
-           << " audio stream";
-  audio_stream->set_enabled(enabled, currTime);
+
+  // 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);
+  }
 }
 
 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());
   }
-  DVLOG(1) << __func__ << ": " << (enabled ? "enabling" : "disabling")
-           << " video stream";
-  video_stream->set_enabled(enabled, currTime);
+
+  // 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);
+  }
 }
 
 bool ChunkDemuxer::EvictCodedFrames(const std::string& id,
                                     base::TimeDelta currentMediaTime,
                                     size_t newDataSize) {
   DVLOG(1) << __func__ << "(" << id << ")"
            << " media_time=" << currentMediaTime.InSecondsF()
            << " newDataSize=" << newDataSize;
   base::AutoLock auto_lock(lock_);
 
@@ skipping 333 matching lines @@
   }
 
   return false;
 }
 
 void ChunkDemuxer::OnSourceInitDone(
     const StreamParser::InitParameters& params) {
   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;
   }
 
   if (!params.duration.is_zero() && duration_ == kNoTimestamp)
     UpdateDuration(params.duration);
 
   if (!params.timeline_offset.is_null()) {
     if (!timeline_offset_.is_null() &&
         params.timeline_offset != timeline_offset_) {
       MEDIA_LOG(ERROR, media_log_)
           << "Timeline offset is not the same across all SourceBuffers.";
       ReportError_Locked(DEMUXER_ERROR_COULD_NOT_OPEN);

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

This patchset allows (see above) AddId(z) + appendData(z) -> OnSourceInitDone(z)
to occur *after* demuxer has been successfully opened. This error seems wrong,
then, though some error should occur if timeline_offset_ differs in this
scenario.

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

Nope, see my explanation in the other comment. AddId will fail if the demuxer
has been opened (i.e. fired the init_cb_ and transitioned to INITIALIZED state)
already due to a check at
https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14731...
Btw, I have actually replaced the old layout test that was making an assumption
about more than ~20 SourceBuffers being supported with a test that tries to add
a SourceBuffer after initializing existing SourceBuffers - that also results in
the QuotaExceeded exception being thrown (see the changes in
mediasource-addsourcebuffer.html in this CL)

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

Acknowledged.

       return;
     }
 
     timeline_offset_ = params.timeline_offset;
   }
 
   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;
   detected_video_track_count_ += params.detected_video_track_count;
   detected_text_track_count_ += params.detected_text_track_count;
 
   // Wait until all streams have initialized.
   pending_source_init_done_count_--;

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

See comment above. Once we've reached 0 here, the count may increase again and a
later OnSourceInitDone() may cause it to reach 0 again, triggering bad behavior:
over-logging of track counts, rappor videocodec, seeking everything back to 0,
and attempting to rerun a now-null init_cb.

Another bad behavior (never reaching 0 here) may occur if a RemoveId() occurs
after AddId and before the associated OnSourceInitDone() has occurred.

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

Yep, as I've explained above I believe we can solve this but getting rid of the
counter and keeping track of which ids have been initialized via a map<id, bool
is_initialized>. But note that this is not a regression from the current
behavior!

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

Ack.

 
   if (pending_source_init_done_count_ > 0)
     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.
   UMA_HISTOGRAM_COUNTS_100("Media.MSE.DetectedTrackCount.Audio",
                            detected_audio_track_count_);
   UMA_HISTOGRAM_COUNTS_100("Media.MSE.DetectedTrackCount.Video",
                            detected_video_track_count_);
   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());
   StartReturningData();
 
   if (duration_ == kNoTimestamp)
     duration_ = kInfiniteDuration;
 
   // The demuxer is now initialized after the |start_timestamp_| was set.
   ChangeState_Locked(INITIALIZED);
   base::ResetAndReturn(&init_cb_).Run(PIPELINE_OK);
 }
 
 // static
 MediaTrack::Id ChunkDemuxer::GenerateMediaTrackId() {
   static unsigned g_track_count = 0;
   return base::UintToString(++g_track_count);
 }
 
 ChunkDemuxerStream* ChunkDemuxer::CreateDemuxerStream(
     DemuxerStream::Type type) {
   // New ChunkDemuxerStreams can be created only during initialization segment
   // processing, which happens when a new chunk of data is appended and the
   // lock_ must be held by ChunkDemuxer::AppendData.
   lock_.AssertAcquired();
 
   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);
       break;
     }
     case DemuxerStream::UNKNOWN:
     case DemuxerStream::NUM_TYPES:
       NOTREACHED();
       return NULL;
   }
@@ skipping 107 matching lines @@
 }
 
 void ChunkDemuxer::ShutdownAllStreams() {
   for (MediaSourceStateMap::iterator itr = source_state_map_.begin();
        itr != source_state_map_.end(); ++itr) {
     itr->second->Shutdown();
   }
 }
 
 }  // namespace media