Serialize usage of SurfaceView between AVDA instances.

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 526146156f8b64b473e3b66fa7f4237cf75d648a..40836c7f2070c8e13d89e9b5ae2a5f48baf5290b 100644
--- a/media/gpu/android_video_decode_accelerator.cc
+++ b/media/gpu/android_video_decode_accelerator.cc
@@ -249,6 +249,65 @@ class AVDATimerManager {
     return construction_thread_.task_runner();
   }
 
+  // |owner| would like to use |surface_id|.  If it is not busy, then mark it

[watk, 2016/05/18 20:38:18]

No "owner" any more

[liberato (no reviews please), 2016/05/18 21:13:13]

Done.

+  // as busy and return true.  If it is busy, then replace any existing waiter,
+  // make |owner| the current waiter, and return false.  Any existing waiter
+  // is assumed to be on the way out, so we fail its allocation request.
+  bool AllocateSurface(int surface_id, AndroidVideoDecodeAccelerator* avda) {
+    SurfaceWaiterMap::iterator iter = surface_waiter_map_.find(surface_id);
+
+    // Nobody has to wait for no surface.

[DaleCurtis, 2016/05/18 17:59:11]

Move above find?

[liberato (no reviews please), 2016/05/18 18:49:39]

Done.

+    if (surface_id == AndroidVideoDecodeAccelerator::Config::kNoSurfaceID)
+      return true;
+
+    if (iter == surface_waiter_map_.end()) {
+      // SurfaceView isn't allocated.  Succeed.
+      surface_waiter_map_[surface_id].owner = avda;
+      surface_waiter_map_[surface_id].waiter = nullptr;
+      return true;
+    }
+
+    // SurfaceView is already allocated.
+
+    if (iter->second.waiter != nullptr) {

[DaleCurtis, 2016/05/18 17:59:11]

Can you explain this design choice? I.e. why would we want to do this versus a
queue of waiters?

[liberato (no reviews please), 2016/05/18 18:49:39]

because WMPI maintains at most one AVDA instance.  if the surface id matches,
then the most recent one should win and the others lose.

[DaleCurtis, 2016/05/18 18:55:49]

Hmm, what about other WMPI instances?

[liberato (no reviews please), 2016/05/18 19:51:18]

|surface_id_| will not be the same if the surface is in use anywhere.  in fact,
it might just never be re-used.

+      // Some other AVDA is waiting.  |avda| will replace it, so notify it
+      // that it will fail.
+      iter->second.waiter->OnSurfaceAvailable(false);
+      iter->second.waiter = nullptr;
+    }
+
+    // |avda| is now waiting.
+    iter->second.waiter = avda;
+    return false;
+  }
+
+  // Clear any waiting request for |surface_id| by |avda|.  It is okay if
+  // |waiter| is not waiting and/or isn't the owner of |surface_id|.
+  void DeallocateSurface(int surface_id, AndroidVideoDecodeAccelerator* avda) {
+    SurfaceWaiterMap::iterator iter = surface_waiter_map_.find(surface_id);
+    if (iter == surface_waiter_map_.end())

[DaleCurtis, 2016/05/18 17:59:11]

Should this dcheck?

[liberato (no reviews please), 2016/05/18 18:49:39]

no.  this way, one simply calls Deallocate unconditionally.  otherwise, one has
to track whether one is a waiter or not inside AVDA.

+      return;
+
+    // If |avda| was waiting, then remove it without OnSurfaceAvailable.
+    if (iter->second.waiter == avda)
+      iter->second.waiter = nullptr;
+
+    // If |avda| is the owner, then let the next owner have it.
+    if (iter->second.owner == avda) {

[DaleCurtis, 2016/05/18 17:59:11]

Use early returns instead.

[liberato (no reviews please), 2016/05/18 18:49:39]

Done.

+      if (AndroidVideoDecodeAccelerator* waiter = iter->second.waiter) {
+        // Promote |waiter| to be the owner.
+        iter->second.owner = waiter;
+        iter->second.waiter = nullptr;
+        waiter->OnSurfaceAvailable(true);
+      } else {
+        // No waiter -- remove the record and return since |iter| is
+        // no longer valid.
+        surface_waiter_map_.erase(iter);
+        return;
+      }
+    }
+  }
+
  private:
   friend struct base::DefaultLazyInstanceTraits<AVDATimerManager>;
 
@@ -280,6 +339,15 @@ class AVDATimerManager {
   // All AVDA instances that might like to use the construction thread.
   std::set<AndroidVideoDecodeAccelerator*> thread_avda_instances_;
 
+  // [surface id] = accelerator that owns the surface, or nullptr.

[watk, 2016/05/18 20:38:18]

Comment seems out of date/wrong place

[liberato (no reviews please), 2016/05/18 21:13:13]

Done.

+  struct OwnerRecord {
+    OwnerRecord() : owner(nullptr), waiter(nullptr) {}

[DaleCurtis, 2016/05/18 17:59:11]

Constructor prevents this from being a POD type. I.e. you can't do a = {null,
null}.

[liberato (no reviews please), 2016/05/18 18:49:39]

hrm, i just tried brace-or-equal but that doesn't allow pod initialization
either.  i've switched to that because it looks nicer.

if i can only get one, then i'd rather have nullptr initializers, since this is
used in a map.

is there a way to get both, without defining an unused two-arg constructor (if
that even works)?

[DaleCurtis, 2016/05/18 18:55:49]

Dropping the constructor and not using = nullptr on the members is the only way
to get a POD type. If you want null initialization (no reason for a map use case
where you're looking up keys), then just use the = nullptr syntax instead of a
constructor at least.

[liberato (no reviews please), 2016/05/18 19:51:17]

i switched to "= nullptr" in the most recent CL.  even for maps, i'd like to
guarantee that the ptrs are initialized when adding a key.

+    AndroidVideoDecodeAccelerator* owner;
+    AndroidVideoDecodeAccelerator* waiter;
+  };
+  using SurfaceWaiterMap = std::map<int, OwnerRecord>;
+  SurfaceWaiterMap surface_waiter_map_;
+
   // Since we can't delete while iterating when using a set, defer erasure until
   // after iteration complete.
   bool timer_running_ = false;
@@ -318,6 +386,7 @@ AndroidVideoDecodeAccelerator::AndroidVideoDecodeAccelerator(
       error_sequence_token_(0),
       defer_errors_(false),
       deferred_initialization_pending_(false),
+      cdm_id_(0),
       surface_id_(media::VideoDecodeAccelerator::Config::kNoSurfaceID),
       weak_this_factory_(this) {}
 
@@ -359,6 +428,7 @@ bool AndroidVideoDecodeAccelerator::Initialize(const Config& config,
   codec_config_->initial_expected_coded_size_ =
       config.initial_expected_coded_size;
   is_encrypted_ = config.is_encrypted;
+  cdm_id_ = config.cdm_id;
 
   // We signalled that we support deferred initialization, so see if the client
   // does also.
@@ -415,6 +485,33 @@ bool AndroidVideoDecodeAccelerator::Initialize(const Config& config,
   }
 
   surface_id_ = config.surface_id;
+  if (g_avda_timer.Pointer()->AllocateSurface(surface_id_, this)) {
+    // We have succesfully owned the surface, so finish initialization now.
+    return InitializeStrategy();
+  }
+
+  // We have to wait for some other AVDA instance to free up the surface.
+  // OnSurfaceAvailable will be called when it's available.
+  return true;
+}
+
+void AndroidVideoDecodeAccelerator::OnSurfaceAvailable(bool success) {
+  DCHECK(deferred_initialization_pending_);
+
+  if (!success) {
+    // Surface isn't available and/or we're being destroyed.
+    NotifyInitializationComplete(false);
+    deferred_initialization_pending_ = false;
+    return;
+  }
+
+  if (!InitializeStrategy()) {

[watk, 2016/05/18 20:38:18]

Could merge this block into the above with a || if you want

[liberato (no reviews please), 2016/05/18 21:13:13]

Done.

+    NotifyInitializationComplete(false);
+    deferred_initialization_pending_ = false;
+  }
+}
+
+bool AndroidVideoDecodeAccelerator::InitializeStrategy() {
   codec_config_->surface_ = strategy_->Initialize(surface_id_);
   if (codec_config_->surface_.IsEmpty()) {
     LOG(ERROR) << "Failed to initialize the backing strategy. The returned "
@@ -445,7 +542,7 @@ bool AndroidVideoDecodeAccelerator::Initialize(const Config& config,
 
   // If we are encrypted, then we aren't able to create the codec yet.
   if (is_encrypted_) {
-    InitializeCdm(config.cdm_id);
+    InitializeCdm();
     return true;
   }
 
@@ -455,7 +552,11 @@ bool AndroidVideoDecodeAccelerator::Initialize(const Config& config,
   }
 
   // If the client doesn't support deferred initialization (WebRTC), then we
-  // should complete it now and return a meaningful result.
+  // should complete it now and return a meaningful result.  Note that it would
+  // be nice if we didn't have to worry about starting codec configuration at
+  // all (::Initialize or the wrapper can do it), but then they have to remember
+  // not to start codec config if we have to wait for the cdm.  It's somewhat
+  // clearer for us to handle both cases.
   return ConfigureMediaCodecSynchronously();
 }
 
@@ -1204,6 +1305,10 @@ void AndroidVideoDecodeAccelerator::ActualDestroy() {
         on_destroying_surface_cb_);
   }
 
+  // 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(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.
@@ -1296,15 +1401,16 @@ void AndroidVideoDecodeAccelerator::PostError(
   state_ = ERROR;
 }
 
-void AndroidVideoDecodeAccelerator::InitializeCdm(int cdm_id) {
-  DVLOG(2) << __FUNCTION__ << ": " << cdm_id;
+void AndroidVideoDecodeAccelerator::InitializeCdm() {
+  DVLOG(2) << __FUNCTION__ << ": " << cdm_id_;
 
 #if !defined(ENABLE_MOJO_MEDIA_IN_GPU_PROCESS)
   NOTIMPLEMENTED();
   NotifyInitializationComplete(false);
 #else
   // Store the CDM to hold a reference to it.
-  cdm_for_reference_holding_only_ = media::MojoCdmService::LegacyGetCdm(cdm_id);
+  cdm_for_reference_holding_only_ =
+      media::MojoCdmService::LegacyGetCdm(cdm_id_);
   DCHECK(cdm_for_reference_holding_only_);
 
   // On Android platform the CdmContext must be a MediaDrmBridgeCdmContext.