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/1167742Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581672}

content/renderer/media_recorder/vea_encoder.cc

@@ -75,6 +75,11 @@ VEAEncoder::VEAEncoder(
 }
 
 VEAEncoder::~VEAEncoder() {
+  if (encoding_task_runner_->BelongsToCurrentThread()) {
+    DestroyOnEncodingTaskRunner();
+    return;
+  }
+
   base::WaitableEvent release_waiter(
       base::WaitableEvent::ResetPolicy::MANUAL,
       base::WaitableEvent::InitialState::NOT_SIGNALED);
@@ -91,6 +96,13 @@ 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) {
@@ -276,7 +288,8 @@ void VEAEncoder::DestroyOnEncodingTaskRunner(
     base::WaitableEvent* async_waiter) {
   DCHECK(encoding_task_runner_->BelongsToCurrentThread());
   video_encoder_.reset();
-  async_waiter->Signal();
+  if (async_waiter)
+    async_waiter->Signal();
 }
 
 }  // namespace content

content/renderer/media_recorder/vea_encoder.h

@@ -65,12 +65,13 @@ 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);
+  void DestroyOnEncodingTaskRunner(base::WaitableEvent* async_waiter = nullptr);
 
   media::GpuVideoAcceleratorFactories* const gpu_factories_;
 

content/renderer/media_recorder/video_track_recorder.cc

@@ -163,6 +163,23 @@ 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,
@@ -173,7 +190,7 @@ VideoTrackRecorder::Encoder::Encoder(
       paused_(false),
       on_encoded_video_callback_(on_encoded_video_callback),
       bits_per_second_(bits_per_second),
-      num_frames_in_encode_(0) {
+      num_frames_in_encode_(std::make_unique<VideoTrackRecorder::Counter>()) {
   DCHECK(!on_encoded_video_callback_.is_null());
   if (encoding_task_runner_)
     return;
@@ -184,6 +201,10 @@ 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(
@@ -204,7 +225,7 @@ void VideoTrackRecorder::Encoder::StartFrameEncode(
     return;
   }
 
-  if (num_frames_in_encode_ > kMaxNumberOfFramesInEncode) {
+  if (num_frames_in_encode_->count() > kMaxNumberOfFramesInEncode) {
     DLOG(WARNING) << "Too many frames are queued up. Dropping this one.";
     return;
   }
@@ -226,9 +247,13 @@ void VideoTrackRecorder::Encoder::StartFrameEncode(
         video_frame, video_frame->format(), video_frame->visible_rect(),
         video_frame->natural_size());
   }
-  wrapped_frame->AddDestructionObserver(media::BindToCurrentLoop(base::Bind(
-      &VideoTrackRecorder::Encoder::FrameReleased, this, video_frame)));
-  ++num_frames_in_encode_;
+  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();
 
   encoding_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&Encoder::EncodeOnEncodingTaskRunner, this,
@@ -349,12 +374,6 @@ 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();
@@ -377,7 +396,7 @@ VideoTrackRecorder::VideoTrackRecorder(
     int32_t bits_per_second,
     scoped_refptr<base::SingleThreadTaskRunner> main_task_runner)
     : track_(track),
-      paused_before_init_(false),
+      should_pause_encoder_on_initialization_(false),
       main_task_runner_(std::move(main_task_runner)),
       weak_ptr_factory_(this) {
   DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
@@ -407,7 +426,7 @@ void VideoTrackRecorder::Pause() {
   if (encoder_)
     encoder_->SetPaused(true);
   else
-    paused_before_init_ = true;
+    should_pause_encoder_on_initialization_ = true;
 }
 
 void VideoTrackRecorder::Resume() {
@@ -415,7 +434,7 @@ void VideoTrackRecorder::Resume() {
   if (encoder_)
     encoder_->SetPaused(false);
   else
-    paused_before_init_ = false;
+    should_pause_encoder_on_initialization_ = false;
 }
 
 void VideoTrackRecorder::OnVideoFrameForTesting(
@@ -478,9 +497,10 @@ void VideoTrackRecorder::InitializeEncoder(
         NOTREACHED() << "Unsupported codec " << static_cast<int>(codec);
     }
   }
+  encoder_->Initialize(input_size);
 
-  if (paused_before_init_)
-    encoder_->SetPaused(paused_before_init_);
+  if (should_pause_encoder_on_initialization_)
+    encoder_->SetPaused(should_pause_encoder_on_initialization_);
 
   // StartFrameEncode() will be called on Render IO thread.
   MediaStreamVideoSink::ConnectToTrack(

content/renderer/media_recorder/video_track_recorder.h

@@ -69,6 +69,22 @@ 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
@@ -91,6 +107,11 @@ 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
@@ -118,6 +139,13 @@ 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(
@@ -150,7 +178,8 @@ class CONTENT_EXPORT VideoTrackRecorder : public MediaStreamVideoSink {
     const int32_t bits_per_second_;
 
     // Number of frames that we keep the reference alive for encode.
-    uint32_t num_frames_in_encode_;
+    // Operated and released exclusively on |origin_task_runner_|.
+    std::unique_ptr<Counter> num_frames_in_encode_;
 
     // Used to retrieve incoming opaque VideoFrames (i.e. VideoFrames backed by
     // textures). Created on-demand on |main_task_runner_|.
@@ -203,8 +232,7 @@ class CONTENT_EXPORT VideoTrackRecorder : public MediaStreamVideoSink {
                       base::TimeTicks capture_time)>
       initialize_encoder_callback_;
 
-  // Used to track the paused state during the initialization process.
-  bool paused_before_init_;
+  bool should_pause_encoder_on_initialization_;
 
   scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
 

content/renderer/media_recorder/video_track_recorder_unittest.cc

@@ -17,6 +17,7 @@
 #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"
@@ -92,6 +93,9 @@ 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) {
@@ -137,7 +141,7 @@ class VideoTrackRecorderTest
   }
 
   uint32_t NumFramesInEncode() {
-    return video_track_recorder_->encoder_->num_frames_in_encode_;
+    return video_track_recorder_->encoder_->num_frames_in_encode_->count();
   }
 
   // A ChildProcess is needed to fool the Tracks and Sources into believing they