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

content/renderer/media/recorder/media_recorder_handler.cc

 // Copyright 2015 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 "content/renderer/media/recorder/media_recorder_handler.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/location.h"
@@ skipping 40 matching lines @@
   return media::kUnknownVideoCodec;
 }
 
 }  // anonymous namespace
 
 MediaRecorderHandler::MediaRecorderHandler()
     : video_bits_per_second_(0),
       audio_bits_per_second_(0),
       codec_id_(VideoTrackRecorder::CodecId::VP8),
       recording_(false),
+      media_stream_num_tracks_(0),
       client_(nullptr),
       weak_factory_(this) {}
 
 MediaRecorderHandler::~MediaRecorderHandler() {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   // Send a |last_in_slice| to our |client_|.
   if (client_)
     client_->writeData(
         nullptr, 0u, true,
         (TimeTicks::Now() - TimeTicks::UnixEpoch()).InMillisecondsF());
@@ skipping 68 matching lines @@
   else if (codecs_str.find("avc1") != std::string::npos)
     codec_id_ = VideoTrackRecorder::CodecId::H264;
 #endif
   else
     codec_id_ = VideoTrackRecorder::GetPreferredCodecId();
 
   DVLOG_IF(1, codecs_str.empty()) << "Falling back to preferred codec id "
                                   << 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;
 
   audio_bits_per_second_ = audio_bits_per_second;
   video_bits_per_second_ = video_bits_per_second;
   return true;
 }
 
 bool MediaRecorderHandler::start(int timeslice) {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   DCHECK(!recording_);
   DCHECK(!media_stream_.isNull());
   DCHECK(timeslice_.is_zero());
   DCHECK(!webm_muxer_);
 
   timeslice_ = TimeDelta::FromMilliseconds(timeslice);
   slice_origin_timestamp_ = TimeTicks::Now();
 
   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.";
     return false;
   }
 
   const bool use_video_tracks =
       !video_tracks.isEmpty() && video_tracks[0].isEnabled() &&
       video_tracks[0].source().getReadyState() !=
           blink::WebMediaStreamSource::ReadyStateEnded;
@@ skipping 50 matching lines @@
   }
 
   recording_ = true;
   return true;
 }
 
 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();
   audio_recorders_.clear();
   webm_muxer_.reset();
 }
 
 void MediaRecorderHandler::pause() {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   DCHECK(recording_);
@@ skipping 15 matching lines @@
     audio_recorder->Resume();
   webm_muxer_->Resume();
 }
 
 void MediaRecorderHandler::OnEncodedVideo(
     const media::WebmMuxer::VideoParameters& params,
     std::unique_ptr<std::string> encoded_data,
     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,
                                    is_key_frame)) {
     DLOG(ERROR) << "Error muxing video data";
     client_->onError("Error muxing video data");
   }
 }
 
 void MediaRecorderHandler::OnEncodedAudio(
     const media::AudioParameters& params,
     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),
                                    timestamp)) {
     DLOG(ERROR) << "Error muxing audio data";
     client_->onError("Error muxing audio data");
   }
 }
 
 void MediaRecorderHandler::WriteData(base::StringPiece data) {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   const TimeTicks now = TimeTicks::Now();
   // Non-buffered mode does not need to check timestamps.
   if (timeslice_.is_zero()) {
     client_->writeData(data.data(), data.length(), true /* lastInSlice */,
                        (now - TimeTicks::UnixEpoch()).InMillisecondsF());
     return;
   }
 
   const bool last_in_slice = now > slice_origin_timestamp_ + timeslice_;
   DVLOG_IF(1, last_in_slice) << "Slice finished @ " << now;
   if (last_in_slice)
     slice_origin_timestamp_ = now;
   client_->writeData(data.data(), data.length(), last_in_slice,
                      (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) {
   for (const auto& recorder : video_recorders_)
     recorder->OnVideoFrameForTesting(frame, timestamp);
 }
 
 void MediaRecorderHandler::OnAudioBusForTesting(
     const media::AudioBus& audio_bus,
     const base::TimeTicks& timestamp) {
   for (const auto& recorder : audio_recorders_)
     recorder->OnData(audio_bus, timestamp);
 }
 
 void MediaRecorderHandler::SetAudioFormatForTesting(
     const media::AudioParameters& params) {
   for (const auto& recorder : audio_recorders_)
     recorder->OnSetFormat(params);
 }
 
 }  // namespace content