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

This reverts commit 5f6e2aac.

Reason for revert: roll-forward with invalidatino fix and test.

Original change's description:
> Revert "[MediaRecorder] Fix corrupted video problem with pause/resume spamming."
>
> This reverts commit b7dab9dc.
>
> Reason for revert: caused crbug.com/1065827 which is more severe.
>
> Original change's description:
> > [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}
>
> TBR=armax@chromium.org,handellm@google.com
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Change-Id: Id8a148253433576469cd7958addf48afdabf44eb
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2128070
> Reviewed-by: Armando Miraglia <armax@chromium.org>
> Commit-Queue: Armando Miraglia <armax@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#754872}

TBR=armax@chromium.org,handellm@google.com

Change-Id: Ie1becac31f0f1a2e5aea92bc59e7e356a2c6e62b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2129534Reviewed-by: Armando Miraglia <armax@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Armando Miraglia <armax@chromium.org>
Cr-Commit-Position: refs/heads/master@{#755294}

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

@@ -239,6 +239,8 @@ bool MediaRecorderHandler::Start(int timeslice) {
   DCHECK(timeslice_.is_zero());
   DCHECK(!webm_muxer_);
 
+  invalidated_ = false;
+
   timeslice_ = base::TimeDelta::FromMilliseconds(timeslice);
   slice_origin_timestamp_ = base::TimeTicks::Now();
 
@@ -335,8 +337,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 +497,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 +529,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 +559,7 @@ void MediaRecorderHandler::OnEncodedAudio(const media::AudioParameters& params,
                                           base::TimeTicks timestamp) {
   DCHECK(IsMainThread());
 
-  if (audio_recorders_.IsEmpty())
+  if (invalidated_)
     return;
 
   if (UpdateTracksAndCheckIfChanged()) {
@@ -576,7 +578,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

@@ -139,6 +139,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());
@@ -507,6 +517,69 @@ 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;
+}
+
+TEST_P(MediaRecorderHandlerTest, StartStopStartRecorderForVideo) {
+  // 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));
+  media_recorder_handler_->Stop();
+
+  Mock::VerifyAndClearExpectations(recorder);
+  EXPECT_TRUE(media_recorder_handler_->Start(0));
+
+  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;
@@ -624,8 +697,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();
 }