Reland #2 [Media Recorder] Fix video freeze on short recording.

Reason for the revert was duplicate initialization that happened because a
different fix for the initialization issue had already landed in the master
branch.

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: I77d6865be574bf86afca7d0908ce2339f6ecd91e
Reviewed-on: https://chromium-review.googlesource.com/1173408Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582753}

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);
@@ -276,7 +281,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

@@ -70,7 +70,7 @@ class VEAEncoder final : public VideoTrackRecorder::Encoder,
 
   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(
@@ -479,8 +498,8 @@ void VideoTrackRecorder::InitializeEncoder(
     }
   }
 
-  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
@@ -118,6 +134,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 +173,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 +227,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