Make AVDA fall back to software decoding if needed.

media/gpu/android_video_decode_accelerator.cc

Index: media/gpu/android_video_decode_accelerator.cc
diff --git a/media/gpu/android_video_decode_accelerator.cc b/media/gpu/android_video_decode_accelerator.cc
index c81728d5969afde50339a696739e33983d062a55..9bf03f043ed1b5bc17983c737d8881733e6fdc9d 100644
--- a/media/gpu/android_video_decode_accelerator.cc
+++ b/media/gpu/android_video_decode_accelerator.cc
@@ -115,10 +115,18 @@ enum FormatChangedValue {
   MissingFormatChanged = true
 };
 
+// Maximum number of concurrent, incomplete codec creations that we'll allow
+// before turning off autodection of codec type.
+enum { kMaxConcurrentCodecAutodetections = 4 };
+
 static inline void RecordFormatChangedMetric(FormatChangedValue value) {
   UMA_HISTOGRAM_BOOLEAN("Media.AVDA.MissingFormatChanged", !!value);
 }
 
+static void delete_codec_helper(std::unique_ptr<VideoCodecBridge> codec) {
+  // Do nothing with codec.  It will be deleted / released on destruction.
+}
+
 // Handle OnFrameAvailable callbacks safely.  Since they occur asynchronously,
 // we take care that the AVDA that wants them still exists.  A WeakPtr to
 // the AVDA would be preferable, except that OnFrameAvailable callbacks can
@@ -185,7 +193,10 @@ class AVDATimerManager {
   bool StartThread(AndroidVideoDecodeAccelerator* avda_instance) {
     DCHECK(thread_checker_.CalledOnValidThread());
 
-    if (thread_avda_instances_.empty()) {
+    // If we chose not to shut it down due to pending codec constructions, then
+    // the thread might already be started even if there are no avda instances.
+    // Plus, sometimes we just fail to start the thread.
+    if (!construction_thread_.IsRunning()) {
       if (!construction_thread_.Start()) {
         LOG(ERROR) << "Failed to start construction thread.";
         return false;
@@ -202,8 +213,17 @@ class AVDATimerManager {
     DCHECK(thread_checker_.CalledOnValidThread());
 
     thread_avda_instances_.erase(avda_instance);
-    if (thread_avda_instances_.empty())
-      construction_thread_.Stop();
+    if (!thread_avda_instances_.empty())
+      return;
+
+    // Don't stop the thread if there are outstanding requests, since they
+    // might be hung.  They also might simply be incomplete, and the thread
+    // will stay running until we try to shut it down again.
+    base::AutoLock auto_lock(autodetection_info_.lock_);
+    if (autodetection_info_.outstanding_)
+      return;
+
+    construction_thread_.Stop();
   }
 
   // Request periodic callback of |avda_instance|->DoIOTask(). Does nothing if
@@ -249,6 +269,29 @@ class AVDATimerManager {
     return construction_thread_.task_runner();
   }
 
+  // Called on the main thread when codec autodetection starts.  There may be
+  // several calls to this before any call to OnAnyThread.

[watk, 2016/06/22 22:39:46]

Can you clarify what OnAnyThread is?

[liberato (no reviews please), 2016/06/23 15:18:20]

meant "OnAsync...Complete", thanks.

+  void OnAsyncCodecAutodetectionStarted() {
+    base::AutoLock auto_lock(autodetection_info_.lock_);
+    ++autodetection_info_.outstanding_;
+  }
+
+  // Called on any thread when a codec is constructed with autodetection.  This
+  // assumes that requests are ordered, so please don't mix sync and async codec
+  // construction here.  This may be called on any thread.
+  void OnAsyncCodecAutodetectionComplete() {
+    base::AutoLock auto_lock_l(autodetection_info_.lock_);
+    DCHECK_GT(autodetection_info_.outstanding_, 0);
+    --autodetection_info_.outstanding_;
+  }
+
+  // Return a hint about whether autodetecting the codec type is safe or not.

[watk, 2016/06/22 22:39:46]

I feel like we're too focused on autodetection as the key concept we're worried
about.

To me the key concept is: is the AVDA thread hung? If it is, we shouldn't use it
any more, and we should use software codecs on the main thread because they
probably don't hang.

Anyway, my 2c. I'm fine with how it is if you choose to leave it.

[liberato (no reviews please), 2016/06/23 15:18:19]

yeah, i can see that.  i chose 'autodetection' because it's the action that one
must avoid, but you're right - one must also avoid posting onto the codec
construction thread if it's hung up.

hrm.  i'll pick whichever name is shorter.  :)

+  bool IsCodecAutodetectionProbablySafe() {
+    base::AutoLock auto_lock_l(autodetection_info_.lock_);
+
+    return autodetection_info_.outstanding_ < kMaxConcurrentCodecAutodetections;
+  }
+
   // |avda| would like to use |surface_id|.  If it is not busy, then mark it
   // as busy and return true.  If it is busy, then replace any existing waiter,
   // make |avda| the current waiter, and return false.  Any existing waiter
@@ -354,6 +397,15 @@ class AVDATimerManager {
   // Repeating timer responsible for draining pending IO to the codecs.
   base::RepeatingTimer io_timer_;
 
+  // Data for determining if codec creation is hanging.
+  struct {
+    // Lock that protects other members of this struct.
+    base::Lock lock_;
+
+    // Number of currently pending autodetection requests.
+    int outstanding_ = 0;
+  } autodetection_info_;
+
   base::Thread construction_thread_;
 
   base::ThreadChecker thread_checker_;
@@ -1047,8 +1099,19 @@ void AndroidVideoDecodeAccelerator::ConfigureMediaCodecAsynchronously() {
     strategy_->CodecChanged(nullptr);
   }
 
+  // Choose whether to autodetect the codec type.
+  codec_config_->allow_autodetection_ =
+      g_avda_timer.Pointer()->IsCodecAutodetectionProbablySafe();
+  codec_config_->notify_completion_ = codec_config_->allow_autodetection_;
+  if (codec_config_->allow_autodetection_)
+    g_avda_timer.Pointer()->OnAsyncCodecAutodetectionStarted();
+
+  // If we're not trying autodetection, then use the main thread.  The original
+  // might be blocked.
   scoped_refptr<base::SingleThreadTaskRunner> task_runner =
-      g_avda_timer.Pointer()->ConstructionTaskRunner();
+      codec_config_->allow_autodetection_
+          ? g_avda_timer.Pointer()->ConstructionTaskRunner()
+          : base::ThreadTaskRunnerHandle::Get();
   CHECK(task_runner);
 
   base::PostTaskAndReplyWithResult(
@@ -1061,6 +1124,15 @@ void AndroidVideoDecodeAccelerator::ConfigureMediaCodecAsynchronously() {
 
 bool AndroidVideoDecodeAccelerator::ConfigureMediaCodecSynchronously() {
   state_ = WAITING_FOR_CODEC;
+
+  // Decide whether to allow autodetection or not.  Since we're on the main
+  // thread, and this request is unordered with respect to pending async config
+  // attempts, don't record it.  It may break book-keeping, and there's not
+  // much we can do anyway.
+  codec_config_->allow_autodetection_ =
+      g_avda_timer.Pointer()->IsCodecAutodetectionProbablySafe();
+  codec_config_->notify_completion_ = false;
+
   std::unique_ptr<VideoCodecBridge> media_codec =
       ConfigureMediaCodecOnAnyThread(codec_config_);
   OnCodecConfigured(std::move(media_codec));
@@ -1079,10 +1151,20 @@ AndroidVideoDecodeAccelerator::ConfigureMediaCodecOnAnyThread(
   // |needs_protected_surface_| implies encrypted stream.
   DCHECK(!codec_config->needs_protected_surface_ || media_crypto);
 
-  return std::unique_ptr<VideoCodecBridge>(VideoCodecBridge::CreateDecoder(
+  const bool require_software = !codec_config->allow_autodetection_;
+
+  std::unique_ptr<VideoCodecBridge> codec(VideoCodecBridge::CreateDecoder(
       codec_config->codec_, codec_config->needs_protected_surface_,
       codec_config->initial_expected_coded_size_,
-      codec_config->surface_.j_surface().obj(), media_crypto, true));
+      codec_config->surface_.j_surface().obj(), media_crypto, true,
+      require_software));
+
+  // If we successfully completed after an autodetect, then reset the timer so

[watk, 2016/06/22 22:39:46]

"reset the timer" feels like a misleading way to say what we're doing :P

[liberato (no reviews please), 2016/06/23 15:18:20]

Done.

+  // that other instances know that autodetection is okay.
+  if (codec_config->notify_completion_)
+    g_avda_timer.Pointer()->OnAsyncCodecAutodetectionComplete();
+
+  return codec;
 }
 
 void AndroidVideoDecodeAccelerator::OnCodecConfigured(
@@ -1317,6 +1399,8 @@ void AndroidVideoDecodeAccelerator::ActualDestroy() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  AVDATimerManager* manager = g_avda_timer.Pointer();

[watk, 2016/06/22 22:39:46]

Move variable closer to usage.

[liberato (no reviews please), 2016/06/23 15:18:19]

Done.

+
   if (!on_destroying_surface_cb_.is_null()) {
     AVDASurfaceTracker::GetInstance()->UnregisterOnDestroyingSurfaceCallback(
         on_destroying_surface_cb_);
@@ -1324,15 +1408,25 @@ void AndroidVideoDecodeAccelerator::ActualDestroy() {
 
   // We no longer care about |surface_id|, in case we did before.  It's okay
   // if we have no surface and/or weren't the owner or a waiter.
-  g_avda_timer.Pointer()->DeallocateSurface(config_.surface_id, this);
+  manager->DeallocateSurface(config_.surface_id, this);
 
   // Note that async codec construction might still be in progress.  In that
   // case, the codec will be deleted when it completes once we invalidate all
   // our weak refs.
   weak_this_factory_.InvalidateWeakPtrs();
   if (media_codec_) {
-    g_avda_timer.Pointer()->StopTimer(this);
-    media_codec_.reset();
+    manager->StopTimer(this);
+    // If codec construction is broken, then we can't release this codec if it's
+    // backed by hardware, else it may hang too.  Post it to the construction
+    // thread, and it'll get freed if things start working.  If things are
+    // already working, then it'll be freed soon.
+    if (media_codec_->IsSoftwareCodec()) {
+      media_codec_.reset();
+    } else {
+      manager->ConstructionTaskRunner()->PostTask(

[DaleCurtis, 2016/06/22 21:20:11]

Can you just use DeleteSoon() ?

[liberato (no reviews please), 2016/06/22 21:43:55]

ooh, shiny.

+          FROM_HERE,
+          base::Bind(delete_codec_helper, base::Passed(&media_codec_)));

[watk, 2016/06/22 22:39:46]

We might consider incrementing outstanding_ while a destruction is pending
because it might hang too. Making outstanding_ the number of tasks on the AVDA
thread. i.e., the number of tasks that may never complete.

It's a shame TaskRunner doesn't have a way to query the number of queued tasks.

+    }
   }
   delete this;
 }