Commit 5f6e2aac authored by Armando Miraglia's avatar Armando Miraglia Committed by Commit Bot

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/+/2128070Reviewed-by: default avatarArmando Miraglia <armax@chromium.org>
Commit-Queue: Armando Miraglia <armax@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754872}
parent 6297e667
......@@ -335,7 +335,8 @@ void MediaRecorderHandler::Stop() {
DCHECK(IsMainThread());
// Don't check |recording_| since we can go directly from pause() to stop().
invalidated_ = true;
if (recording_)
Pause();
recording_ = false;
timeslice_ = base::TimeDelta::FromMilliseconds(0);
......@@ -495,10 +496,6 @@ 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),
......@@ -527,6 +524,9 @@ 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 (invalidated_)
if (audio_recorders_.IsEmpty())
return;
if (UpdateTracksAndCheckIfChanged()) {
......@@ -576,7 +576,7 @@ void MediaRecorderHandler::OnEncodedAudio(const media::AudioParameters& params,
void MediaRecorderHandler::WriteData(base::StringPiece data) {
DCHECK(IsMainThread());
if (invalidated_)
if (!recording_)
return;
const base::TimeTicks now = base::TimeTicks::Now();
......
......@@ -144,7 +144,6 @@ 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_;
......
......@@ -139,16 +139,6 @@ 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());
......@@ -517,37 +507,6 @@ 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;
......@@ -665,8 +624,8 @@ TEST_F(MediaRecorderHandlerPassthroughTest, ErrorsOutOnCodecSwitch) {
// transfer doesn't crash the media recorder.
OnVideoFrameForTesting(FakeEncodedVideoFrame::Builder()
.WithKeyFrame(true)
.WithCodec(media::kCodecVP8)
.WithData(std::string("vp8 frame"))
.WithCodec(media::kCodecVP9)
.WithData(std::string("vp9 frame"))
.BuildRefPtr());
platform_->RunUntilIdle();
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment