Revert "Reland [Media Recorder] Fix video freeze on short recording. Fix tab crash on initialize."

This reverts commit 545410e6.

Reason for revert: A different fix for the initialization had already landed by now, see https://chromium-review.googlesource.com/c/chromium/src/+/1158316. I need to remove the extra initialization changes from my CL.

Original change's description:
> Reland [Media Recorder] Fix video freeze on short recording. Fix tab crash on initialize.
> 
> Patch Set 1 is the original CL that got reverted.
> Patch Set 2 fixes the reason for the revert.
> 
> Reason for the revert was a memory leak during a unit test. This was caused by
> a newly added DeleteSoon() causing the test to exist before all objects had
> been released. The fix is to add a ScopedTaskEnvironment::RunUntilIdle() at
> test teardown.
> 
> Original CL description:
> 
> Fixes an issue where VideoTrackRecorder when used in combination with
> VEAEncoder and very short recording durations (just 1 frame) would
> occasionally hold on to video frames forever, causing frame buffers
> from the buffer pool to become permanently blocked, eventually leading
> to video capture freezing.
> 
> The cause was a circular ownership that would happen if only 1 frame
> is recorded.
> 
> This CL additionally fixes a crash that could happen on initialization
> of VEAEncoder. This issue was revealed during testing of the above fix.
> Since it is not 100% if this crash was masked by the other issue, and
> would only start occurring with the above fix, I am making a fix for that
> part of this CL as well.
> 
> The cause for the crash issue was that class VEAEncoder, which uses
> RefCountedThreadSafe<>, would post an asynchronous task from its
> constructor. If this task would finish executing before the newly
> constructed instance was assigned to a scoped_refptr<> by however
> called the constructor, the instance would destroy itself, because
> of the AddRef() and subsequent Release() done by the posting and
> running of the asynchronous task.
> 
> The fix for this is to use a new method Initialize() instead of
> posting a task from the constructor.
> 
> TBR=emircan@chromium.org
> 
> Bug: 859610
> Change-Id: I120b60b4b5cdf7462e38a74b7c757af6b5e1a7d6
> Reviewed-on: https://chromium-review.googlesource.com/1167742
> Reviewed-by: Christian Fremerey <chfremer@chromium.org>
> Commit-Queue: Christian Fremerey <chfremer@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#581672}

TBR=emircan@chromium.org,chfremer@chromium.org

Change-Id: I4794d8d76b78bd1ab86860230bc01f0f20d3bd18
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 859610
Reviewed-on: https://chromium-review.googlesource.com/1167768Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581679}

content/renderer/media_recorder/vea_encoder.cc

@@ -75,11 +75,6 @@ VEAEncoder::VEAEncoder(
 }
 
 VEAEncoder::~VEAEncoder() {
-  if (encoding_task_runner_->BelongsToCurrentThread()) {
-    DestroyOnEncodingTaskRunner();
-    return;
-  }
-
   base::WaitableEvent release_waiter(
       base::WaitableEvent::ResetPolicy::MANUAL,
       base::WaitableEvent::InitialState::NOT_SIGNALED);
@@ -96,13 +91,6 @@ VEAEncoder::~VEAEncoder() {
   release_waiter.Wait();
 }
 
-void VEAEncoder::Initialize(const gfx::Size& resolution) {
-  encoding_task_runner_->PostTask(
-      FROM_HERE,
-      base::BindOnce(&VEAEncoder::ConfigureEncoderOnEncodingTaskRunner, this,
-                     resolution));
-}
-
 void VEAEncoder::RequireBitstreamBuffers(unsigned int /*input_count*/,
                                          const gfx::Size& input_coded_size,
                                          size_t output_buffer_size) {
@@ -288,8 +276,7 @@ void VEAEncoder::DestroyOnEncodingTaskRunner(
     base::WaitableEvent* async_waiter) {
   DCHECK(encoding_task_runner_->BelongsToCurrentThread());
   video_encoder_.reset();
-  if (async_waiter)
-    async_waiter->Signal();
+  async_waiter->Signal();
 }
 
 }  // namespace content

content/renderer/media_recorder/vea_encoder.h

@@ -65,13 +65,12 @@ class VEAEncoder final : public VideoTrackRecorder::Encoder,
 
   // VideoTrackRecorder::Encoder implementation.
   ~VEAEncoder() override;
-  void Initialize(const gfx::Size& resolution) override;
   void EncodeOnEncodingTaskRunner(scoped_refptr<media::VideoFrame> frame,
                                   base::TimeTicks capture_timestamp) override;
 
   void ConfigureEncoderOnEncodingTaskRunner(const gfx::Size& size);
 
-  void DestroyOnEncodingTaskRunner(base::WaitableEvent* async_waiter = nullptr);
+  void DestroyOnEncodingTaskRunner(base::WaitableEvent* async_waiter);
 
   media::GpuVideoAcceleratorFactories* const gpu_factories_;
 

content/renderer/media_recorder/video_track_recorder.cc

@@ -163,23 +163,6 @@ media::VideoCodecProfile CodecEnumerator::CodecIdToVEAProfile(CodecId codec) {
 
 }  // anonymous namespace
 
-VideoTrackRecorder::Counter::Counter() : count_(0u), weak_factory_(this) {}
-
-VideoTrackRecorder::Counter::~Counter() = default;
-
-void VideoTrackRecorder::Counter::IncreaseCount() {
-  count_++;
-}
-
-void VideoTrackRecorder::Counter::DecreaseCount() {
-  count_--;
-}
-
-base::WeakPtr<VideoTrackRecorder::Counter>
-VideoTrackRecorder::Counter::GetWeakPtr() {
-  return weak_factory_.GetWeakPtr();
-}
-
 VideoTrackRecorder::Encoder::Encoder(
     const OnEncodedVideoCB& on_encoded_video_callback,
     int32_t bits_per_second,
@@ -190,7 +173,7 @@ VideoTrackRecorder::Encoder::Encoder(
       paused_(false),
       on_encoded_video_callback_(on_encoded_video_callback),
       bits_per_second_(bits_per_second),
-      num_frames_in_encode_(std::make_unique<VideoTrackRecorder::Counter>()) {
+      num_frames_in_encode_(0) {
   DCHECK(!on_encoded_video_callback_.is_null());
   if (encoding_task_runner_)
     return;
@@ -201,10 +184,6 @@ VideoTrackRecorder::Encoder::Encoder(
 
 VideoTrackRecorder::Encoder::~Encoder() {
   main_task_runner_->DeleteSoon(FROM_HERE, video_renderer_.release());
-  if (origin_task_runner_ && !origin_task_runner_->BelongsToCurrentThread()) {
-    origin_task_runner_->DeleteSoon(FROM_HERE,
-                                    std::move(num_frames_in_encode_));
-  }
 }
 
 void VideoTrackRecorder::Encoder::StartFrameEncode(
@@ -225,7 +204,7 @@ void VideoTrackRecorder::Encoder::StartFrameEncode(
     return;
   }
 
-  if (num_frames_in_encode_->count() > kMaxNumberOfFramesInEncode) {
+  if (num_frames_in_encode_ > kMaxNumberOfFramesInEncode) {
     DLOG(WARNING) << "Too many frames are queued up. Dropping this one.";
     return;
   }
@@ -247,13 +226,9 @@ void VideoTrackRecorder::Encoder::StartFrameEncode(
         video_frame, video_frame->format(), video_frame->visible_rect(),
         video_frame->natural_size());
   }
-  wrapped_frame->AddDestructionObserver(media::BindToCurrentLoop(
-      base::BindOnce(&VideoTrackRecorder::Counter::DecreaseCount,
-                     num_frames_in_encode_->GetWeakPtr())));
-  wrapped_frame->AddDestructionObserver(
-      base::BindOnce([](const scoped_refptr<VideoFrame>& video_frame) {},
-                     std::move(video_frame)));
-  num_frames_in_encode_->IncreaseCount();
+  wrapped_frame->AddDestructionObserver(media::BindToCurrentLoop(base::Bind(
+      &VideoTrackRecorder::Encoder::FrameReleased, this, video_frame)));
+  ++num_frames_in_encode_;
 
   encoding_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&Encoder::EncodeOnEncodingTaskRunner, this,
@@ -374,6 +349,12 @@ bool VideoTrackRecorder::Encoder::CanEncodeAlphaChannel() {
   return false;
 }
 
+void VideoTrackRecorder::Encoder::FrameReleased(
+    const scoped_refptr<VideoFrame>& frame) {
+  DCHECK(origin_task_runner_->BelongsToCurrentThread());
+  --num_frames_in_encode_;
+}
+
 // static
 VideoTrackRecorder::CodecId VideoTrackRecorder::GetPreferredCodecId() {
   return GetCodecEnumerator()->GetPreferredCodecId();
@@ -396,7 +377,7 @@ VideoTrackRecorder::VideoTrackRecorder(
     int32_t bits_per_second,
     scoped_refptr<base::SingleThreadTaskRunner> main_task_runner)
     : track_(track),
-      should_pause_encoder_on_initialization_(false),
+      paused_before_init_(false),
       main_task_runner_(std::move(main_task_runner)),
       weak_ptr_factory_(this) {
   DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
@@ -426,7 +407,7 @@ void VideoTrackRecorder::Pause() {
   if (encoder_)
     encoder_->SetPaused(true);
   else
-    should_pause_encoder_on_initialization_ = true;
+    paused_before_init_ = true;
 }
 
 void VideoTrackRecorder::Resume() {
@@ -434,7 +415,7 @@ void VideoTrackRecorder::Resume() {
   if (encoder_)
     encoder_->SetPaused(false);
   else
-    should_pause_encoder_on_initialization_ = false;
+    paused_before_init_ = false;
 }
 
 void VideoTrackRecorder::OnVideoFrameForTesting(
@@ -497,10 +478,9 @@ void VideoTrackRecorder::InitializeEncoder(
         NOTREACHED() << "Unsupported codec " << static_cast<int>(codec);
     }
   }
-  encoder_->Initialize(input_size);
 
-  if (should_pause_encoder_on_initialization_)
-    encoder_->SetPaused(should_pause_encoder_on_initialization_);
+  if (paused_before_init_)
+    encoder_->SetPaused(paused_before_init_);
 
   // StartFrameEncode() will be called on Render IO thread.
   MediaStreamVideoSink::ConnectToTrack(

content/renderer/media_recorder/video_track_recorder.h

@@ -69,22 +69,6 @@ class CONTENT_EXPORT VideoTrackRecorder : public MediaStreamVideoSink {
                           bool is_key_frame)>;
   using OnErrorCB = base::Closure;
 
-  // Wraps a counter in a class in order to enable use of base::WeakPtr<>.
-  // See https://crbug.com/859610 for why this was added.
-  class Counter {
-   public:
-    Counter();
-    ~Counter();
-    uint32_t count() const { return count_; }
-    void IncreaseCount();
-    void DecreaseCount();
-    base::WeakPtr<Counter> GetWeakPtr();
-
-   private:
-    uint32_t count_;
-    base::WeakPtrFactory<Counter> weak_factory_;
-  };
-
   // Base class to describe a generic Encoder, encapsulating all actual encoder
   // (re)configurations, encoding and delivery of received frames. This class is
   // ref-counted to allow the MediaStreamVideoTrack to hold a reference to it
@@ -107,11 +91,6 @@ class CONTENT_EXPORT VideoTrackRecorder : public MediaStreamVideoSink {
             scoped_refptr<base::SingleThreadTaskRunner> encoding_task_runner =
                 nullptr);
 
-    // This must be called exactly once after the constructor and before any of
-    // the other public methods. Subclasses may choose to override this to
-    // perform initialization work that cannot be done in the constructor.
-    virtual void Initialize(const gfx::Size& resolution) {}
-
     // Start encoding |frame|, returning via |on_encoded_video_callback_|. This
     // call will also trigger an encode configuration upon first frame arrival
     // or parameter change, and an EncodeOnEncodingTaskRunner() to actually
@@ -139,13 +118,6 @@ class CONTENT_EXPORT VideoTrackRecorder : public MediaStreamVideoSink {
     friend class base::RefCountedThreadSafe<Encoder>;
     friend class VideoTrackRecorderTest;
 
-    // This destructor may run on either |main_task_runner|,
-    // |encoding_task_runner|, or |origin_task_runner_|. Main ownership lies
-    // with VideoTrackRecorder. Shared ownership is handed out to
-    // asynchronous tasks running on |encoding_task_runner| for encoding. Shared
-    // ownership is also handed out to a MediaStreamVideoTrack which pushes
-    // frames on |origin_task_runner_|. Each of these may end up being the last
-    // reference.
     virtual ~Encoder();
 
     virtual void EncodeOnEncodingTaskRunner(
@@ -178,8 +150,7 @@ class CONTENT_EXPORT VideoTrackRecorder : public MediaStreamVideoSink {
     const int32_t bits_per_second_;
 
     // Number of frames that we keep the reference alive for encode.
-    // Operated and released exclusively on |origin_task_runner_|.
-    std::unique_ptr<Counter> num_frames_in_encode_;
+    uint32_t num_frames_in_encode_;
 
     // Used to retrieve incoming opaque VideoFrames (i.e. VideoFrames backed by
     // textures). Created on-demand on |main_task_runner_|.
@@ -232,7 +203,8 @@ class CONTENT_EXPORT VideoTrackRecorder : public MediaStreamVideoSink {
                       base::TimeTicks capture_time)>
       initialize_encoder_callback_;
 
-  bool should_pause_encoder_on_initialization_;
+  // Used to track the paused state during the initialization process.
+  bool paused_before_init_;
 
   scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
 

content/renderer/media_recorder/video_track_recorder_unittest.cc

@@ -17,7 +17,6 @@
 #include "base/single_thread_task_runner.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/scoped_task_environment.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "content/child/child_process.h"
 #include "content/renderer/media/stream/media_stream_video_track.h"
 #include "content/renderer/media/stream/mock_media_stream_video_source.h"
@@ -93,9 +92,6 @@ class VideoTrackRecorderTest
     blink_source_.Reset();
     video_track_recorder_.reset();
     blink::WebHeap::CollectAllGarbageForTesting();
-    // VideoTrackRecorder::Encoder::~Encoder may post a DeleteSoon(), which
-    // may cause ASAN to detect a memory leak if we don't wait.
-    scoped_task_environment_.RunUntilIdle();
   }
 
   void InitializeRecorder(VideoTrackRecorder::CodecId codec) {
@@ -141,7 +137,7 @@ class VideoTrackRecorderTest
   }
 
   uint32_t NumFramesInEncode() {
-    return video_track_recorder_->encoder_->num_frames_in_encode_->count();
+    return video_track_recorder_->encoder_->num_frames_in_encode_;
   }
 
   // A ChildProcess is needed to fool the Tracks and Sources into believing they