MediaRecorder: make sure ondataavailable+onstop events are fired before onerror

content/renderer/media/recorder/media_recorder_handler.cc

Index: content/renderer/media/recorder/media_recorder_handler.cc
diff --git a/content/renderer/media/recorder/media_recorder_handler.cc b/content/renderer/media/recorder/media_recorder_handler.cc
index 3c3d2e8787e48b478f083739b4a9ed65e638d09c..ec557b41777f168e4cfc912e43f8bf5c7c749fa8 100644
--- a/content/renderer/media/recorder/media_recorder_handler.cc
+++ b/content/renderer/media/recorder/media_recorder_handler.cc
@@ -58,6 +58,7 @@ MediaRecorderHandler::MediaRecorderHandler()
       audio_bits_per_second_(0),
       codec_id_(VideoTrackRecorder::CodecId::VP8),
       recording_(false),
+      media_stream_num_tracks_(0),
       client_(nullptr),
       weak_factory_(this) {}
 
@@ -146,6 +147,7 @@ bool MediaRecorderHandler::initialize(
                                   << static_cast<int>(codec_id_);
 
   media_stream_ = media_stream;
+  media_stream_num_tracks_ = 0;  // Will be updated in start();

[emircan, 2017/02/17 00:19:07]

Should we set it to 0 in Stop() as well?

[mcasas, 2017/02/17 01:46:14]

Not really  because after stop(), we'd need to 
have to start() again. Actually it's not needed
to reinitialise it/them here.

   DCHECK(client);
   client_ = client;
 
@@ -167,6 +169,7 @@ bool MediaRecorderHandler::start(int timeslice) {
   blink::WebVector<blink::WebMediaStreamTrack> video_tracks, audio_tracks;
   media_stream_.videoTracks(video_tracks);
   media_stream_.audioTracks(audio_tracks);
+  media_stream_num_tracks_ = video_tracks.size() + audio_tracks.size();
 
   if (video_tracks.isEmpty() && audio_tracks.isEmpty()) {
     LOG(WARNING) << __func__ << ": no media tracks.";
@@ -237,6 +240,7 @@ void MediaRecorderHandler::stop() {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   // Don't check |recording_| since we can go directly from pause() to stop().
 
+  weak_factory_.InvalidateWeakPtrs();
   recording_ = false;
   timeslice_ = TimeDelta::FromMilliseconds(0);
   video_recorders_.clear();
@@ -272,6 +276,11 @@ void MediaRecorderHandler::OnEncodedVideo(
     TimeTicks timestamp,
     bool is_key_frame) {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
+
+  if (CheckAndUpdateAmountOfTracks()) {
+    client_->onError("Amount of tracks in MediaStream has changed.");
+    return;
+  }
   if (!webm_muxer_)
     return;
   if (!webm_muxer_->OnEncodedVideo(params, std::move(encoded_data), timestamp,
@@ -286,6 +295,11 @@ void MediaRecorderHandler::OnEncodedAudio(
     std::unique_ptr<std::string> encoded_data,
     base::TimeTicks timestamp) {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
+
+  if (CheckAndUpdateAmountOfTracks()) {
+    client_->onError("Amount of tracks in MediaStream has changed.");
+    return;
+  }
   if (!webm_muxer_)
     return;
   if (!webm_muxer_->OnEncodedAudio(params, std::move(encoded_data),
@@ -313,6 +327,20 @@ void MediaRecorderHandler::WriteData(base::StringPiece data) {
                      (now - TimeTicks::UnixEpoch()).InMillisecondsF());
 }
 
+bool MediaRecorderHandler::CheckAndUpdateAmountOfTracks() {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());
+
+  blink::WebVector<blink::WebMediaStreamTrack> video_tracks, audio_tracks;
+  media_stream_.videoTracks(video_tracks);
+  media_stream_.audioTracks(audio_tracks);
+  const size_t current_num_tracks = video_tracks.size() + audio_tracks.size();
+  if (media_stream_num_tracks_ == current_num_tracks)

[emircan, 2017/02/17 00:19:07]

What about the case where one is added another is removed?

[mcasas, 2017/02/17 01:46:14]

Hmm good point, modified.

+    return false;
+
+  media_stream_num_tracks_ = current_num_tracks;

[emircan, 2017/02/17 00:19:07]

Is it necessary to update |media_stream_num_tracks_| here? Can you change this
function to only check that number of tracks is different than the initialize?

[mcasas, 2017/02/17 01:46:14]

I still need the update because at the end of the
day from MRHandler we're signalling the error, but
the WebApp might like to continue recording with
the current status (i.e. ignoring the error). You
could perfectly argue that all errors are fatal,
but note that we're not calling stop()...

+  return true;
+}
+
 void MediaRecorderHandler::OnVideoFrameForTesting(
     const scoped_refptr<media::VideoFrame>& frame,
     const TimeTicks& timestamp) {