[MediaRecorder] Fix corrupted video problem with pause/resume spamming.

This CL fixes an issue caused by the following sequence of events:
1. MediaRecorderHandler::Pause(): recording_ = false
2. MediaRecorderHandler::WriteData(): if (!recording_) return

The problem is that the WriteData() invocation was decided to
happen while the recorder was still resumed. In that state, wholes
would be produced which are not tolerated by vpx_encoder, which
interprets those as corrupted.

The fix reverts the semantic of the code to when the class was not under
oilpan and would require manual memory management and restores the
"invalidation" semantic from when the class was using a weak ptr.
In fact, the weak reference would be invalidated hence making the
callbacks no-ops. To keep the same semantic, this CL introduces a
boolean and avoids overloading the meaning of 'recording_'

This should also not require calling Pause() in Stop(), as all callbacks
will not do anything after Stop() is called.

Fixed: 1060358
Change-Id: I88b0aeb10fd13034412f3689648cebd1df40d6bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2106072
Commit-Queue: Armando Miraglia <armax@chromium.org>
Reviewed-by: Markus Handell <handellm@google.com>
Cr-Commit-Position: refs/heads/master@{#752838}

third_party/blink/renderer/modules/mediarecorder/media_recorder_handler.cc

@@ -335,8 +335,7 @@ void MediaRecorderHandler::Stop() {
   DCHECK(IsMainThread());
   // Don't check |recording_| since we can go directly from pause() to stop().
 
-  if (recording_)
-    Pause();
+  invalidated_ = true;
 
   recording_ = false;
   timeslice_ = base::TimeDelta::FromMilliseconds(0);
@@ -496,6 +495,10 @@ void MediaRecorderHandler::OnEncodedVideo(
     base::TimeTicks timestamp,
     bool is_key_frame) {
   DCHECK(IsMainThread());
+
+  if (invalidated_)
+    return;
+
   auto params_with_codec = params;
   params_with_codec.codec = MediaVideoCodecFromCodecId(video_codec_id_);
   HandleEncodedVideo(params_with_codec, std::move(encoded_data),
@@ -524,9 +527,6 @@ void MediaRecorderHandler::HandleEncodedVideo(
     bool is_key_frame) {
   DCHECK(IsMainThread());
 
-  if (video_recorders_.IsEmpty())
-    return;
-
   if (UpdateTracksAndCheckIfChanged()) {
     recorder_->OnError("Amount of tracks in MediaStream has changed.");
     return;
@@ -557,7 +557,7 @@ void MediaRecorderHandler::OnEncodedAudio(const media::AudioParameters& params,
                                           base::TimeTicks timestamp) {
   DCHECK(IsMainThread());
 
-  if (audio_recorders_.IsEmpty())
+  if (invalidated_)
     return;
 
   if (UpdateTracksAndCheckIfChanged()) {
@@ -576,7 +576,7 @@ void MediaRecorderHandler::OnEncodedAudio(const media::AudioParameters& params,
 void MediaRecorderHandler::WriteData(base::StringPiece data) {
   DCHECK(IsMainThread());
 
-  if (!recording_)
+  if (invalidated_)
     return;
 
   const base::TimeTicks now = base::TimeTicks::Now();

third_party/blink/renderer/modules/mediarecorder/media_recorder_handler.h

@@ -144,6 +144,7 @@ class MODULES_EXPORT MediaRecorderHandler final
   // The last seen video codec of the last received encoded video frame.
   base::Optional<media::VideoCodec> last_seen_codec_;
 
+  bool invalidated_ = false;
   bool recording_;
   // The MediaStream being recorded.
   Member<MediaStreamDescriptor> media_stream_;

third_party/blink/renderer/modules/mediarecorder/media_recorder_handler_unittest.cc

@@ -137,6 +137,16 @@ class MediaRecorderHandlerTest : public TestWithParam<MediaRecorderTestParams> {
     media_recorder_handler_->OnVideoFrameForTesting(std::move(frame),
                                                     base::TimeTicks::Now());
   }
+
+  void OnEncodedVideoForTesting(const media::WebmMuxer::VideoParameters& params,
+                                std::string encoded_data,
+                                std::string encoded_alpha,
+                                base::TimeTicks timestamp,
+                                bool is_key_frame) {
+    media_recorder_handler_->OnEncodedVideo(params, encoded_data, encoded_alpha,
+                                            timestamp, is_key_frame);
+  }
+
   void OnAudioBusForTesting(const media::AudioBus& audio_bus) {
     media_recorder_handler_->OnAudioBusForTesting(audio_bus,
                                                   base::TimeTicks::Now());
@@ -505,6 +515,37 @@ TEST_P(MediaRecorderHandlerTest, ActualMimeType) {
   media_recorder_handler_ = nullptr;
 }
 
+TEST_P(MediaRecorderHandlerTest, PauseRecorderForVideo) {
+  // Video-only test: Audio would be very similar.
+  if (GetParam().has_audio)
+    return;
+
+  AddTracks();
+
+  V8TestingScope scope;
+  auto* recorder = MakeGarbageCollected<MockMediaRecorder>(scope);
+
+  const String mime_type(GetParam().mime_type);
+  const String codecs(GetParam().codecs);
+
+  EXPECT_TRUE(media_recorder_handler_->Initialize(
+      recorder, registry_.test_stream(), mime_type, codecs, 0, 0));
+  EXPECT_TRUE(media_recorder_handler_->Start(0));
+
+  Mock::VerifyAndClearExpectations(recorder);
+  media_recorder_handler_->Pause();
+
+  EXPECT_CALL(*recorder, WriteData).Times(AtLeast(1));
+  media::WebmMuxer::VideoParameters params(gfx::Size(), 1, media::kCodecVP9,
+                                           gfx::ColorSpace());
+  OnEncodedVideoForTesting(params, "vp9 frame", "", base::TimeTicks::Now(),
+                           true);
+
+  // Expect a last call on destruction.
+  EXPECT_CALL(*recorder, WriteData(_, _, true, _)).Times(1);
+  media_recorder_handler_ = nullptr;
+}
+
 struct MediaRecorderPassthroughTestParams {
   const char* mime_type;
   media::VideoCodec codec;
@@ -621,8 +662,8 @@ TEST_F(MediaRecorderHandlerPassthroughTest, ErrorsOutOnCodecSwitch) {
   // transfer doesn't crash the media recorder.
   OnVideoFrameForTesting(FakeEncodedVideoFrame::Builder()
                              .WithKeyFrame(true)
-                             .WithCodec(media::kCodecVP9)
-                             .WithData(std::string("vp9 frame"))
+                             .WithCodec(media::kCodecVP8)
+                             .WithData(std::string("vp8 frame"))
                              .BuildRefPtr());
   platform_->RunUntilIdle();
 }