Serialize usage of SurfaceView between AVDA instances.

media/gpu/android_video_decode_accelerator.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/gpu/android_video_decode_accelerator.h"
 
 #include <stddef.h>
 
 #include <memory>
 
@@ skipping 231 matching lines @@
       io_timer_.Stop();
   }
 
   // Eventually, we should run the timer on this thread.  For now, we just keep
   // it as a convenience for construction.
   scoped_refptr<base::SingleThreadTaskRunner> ConstructionTaskRunner() {
     DCHECK(thread_checker_.CalledOnValidThread());
     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>;
 
   AVDATimerManager() : construction_thread_("AVDAThread") {}
   ~AVDATimerManager() { NOTREACHED(); }
 
   void RunTimer() {
     {
       // Call out to all AVDA instances, some of which may attempt to remove
       // themselves from the list during this operation; those removals will be
@@ skipping 11 matching lines @@
     // TODO(dalecurtis): We may want to consider chunking this if task execution
     // takes too long for the combined timer.
   }
 
   // All AVDA instances that would like us to poll DoIOTask.
   std::set<AndroidVideoDecodeAccelerator*> timer_avda_instances_;
 
   // 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;
   std::set<AndroidVideoDecodeAccelerator*> pending_erase_;
 
   // Repeating timer responsible for draining pending IO to the codecs.
   base::RepeatingTimer io_timer_;
 
   base::Thread construction_thread_;
 
@@ skipping 18 matching lines @@
       is_encrypted_(false),
       state_(NO_ERROR),
       picturebuffers_requested_(false),
       drain_type_(DRAIN_TYPE_NONE),
       media_drm_bridge_cdm_context_(nullptr),
       cdm_registration_id_(0),
       pending_input_buf_index_(-1),
       error_sequence_token_(0),
       defer_errors_(false),
       deferred_initialization_pending_(false),
+      cdm_id_(0),
       surface_id_(media::VideoDecodeAccelerator::Config::kNoSurfaceID),
       weak_this_factory_(this) {}
 
 AndroidVideoDecodeAccelerator::~AndroidVideoDecodeAccelerator() {
   DCHECK(thread_checker_.CalledOnValidThread());
   g_avda_timer.Pointer()->StopTimer(this);
   g_avda_timer.Pointer()->StopThread(this);
 
 #if defined(ENABLE_MOJO_MEDIA_IN_GPU_PROCESS)
   if (!media_drm_bridge_cdm_context_)
@@ skipping 21 matching lines @@
     return false;
   }
 
   DCHECK(client);
   client_ = client;
   codec_config_ = new CodecConfig();
   codec_config_->codec_ = VideoCodecProfileToVideoCodec(config.profile);
   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.
   deferred_initialization_pending_ = config.is_deferred_initialization_allowed;
 
   if (is_encrypted_ && !deferred_initialization_pending_) {
     DLOG(ERROR) << "Deferred initialization must be used for encrypted streams";
     return false;
   }
 
@@ skipping 36 matching lines @@
     DVLOG(1) << __FUNCTION__ << ", using copy back strategy.";
     strategy_.reset(new AndroidCopyingBackingStrategy(this));
   }
 
   if (!make_context_current_cb_.Run()) {
     LOG(ERROR) << "Failed to make this decoder's GL context current.";
     return false;
   }
 
   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 "
                   "Java surface is empty.";
     return false;
   }
 
   on_destroying_surface_cb_ =
       base::Bind(&AndroidVideoDecodeAccelerator::OnDestroyingSurface,
                  weak_this_factory_.GetWeakPtr());
@@ skipping 10 matching lines @@
 
   // Start the thread for async configuration, even if we don't need it now.
   // ResetCodecState might rebuild the codec later, for example.
   if (!g_avda_timer.Pointer()->StartThread(this)) {
     LOG(ERROR) << "Failed to start AVDA thread";
     return false;
   }
 
   // If we are encrypted, then we aren't able to create the codec yet.
   if (is_encrypted_) {
-    InitializeCdm(config.cdm_id);
+    InitializeCdm();
     return true;
   }
 
   if (deferred_initialization_pending_) {
     ConfigureMediaCodecAsynchronously();
     return true;
   }
 
   // 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();
 }
 
 void AndroidVideoDecodeAccelerator::DoIOTask(bool start_timer) {
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("media", "AVDA::DoIOTask");
   if (state_ == ERROR || state_ == WAITING_FOR_CODEC ||
       state_ == SURFACE_DESTROYED) {
     return;
   }
@@ skipping 728 matching lines @@
 
 void AndroidVideoDecodeAccelerator::ActualDestroy() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (!on_destroying_surface_cb_.is_null()) {
     AVDASurfaceTracker::GetInstance()->UnregisterOnDestroyingSurfaceCallback(
         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.
   weak_this_factory_.InvalidateWeakPtrs();
   if (media_codec_) {
     g_avda_timer.Pointer()->StopTimer(this);
     media_codec_.reset();
   }
   delete this;
 }
@@ skipping 72 matching lines @@
     const ::tracked_objects::Location& from_here,
     media::VideoDecodeAccelerator::Error error) {
   base::MessageLoop::current()->PostDelayedTask(
       from_here,
       base::Bind(&AndroidVideoDecodeAccelerator::NotifyError,
                  weak_this_factory_.GetWeakPtr(), error, error_sequence_token_),
       (defer_errors_ ? ErrorPostingDelay() : base::TimeDelta()));
   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.
   media_drm_bridge_cdm_context_ = static_cast<media::MediaDrmBridgeCdmContext*>(
       cdm_for_reference_holding_only_->GetCdmContext());
   DCHECK(media_drm_bridge_cdm_context_);
 
   // Register CDM callbacks. The callbacks registered will be posted back to
   // this thread via BindToCurrentLoop.
 
@@ skipping 184 matching lines @@
     if (media::MediaCodecUtil::IsSurfaceViewOutputSupported()) {
       capabilities.flags |= media::VideoDecodeAccelerator::Capabilities::
           SUPPORTS_EXTERNAL_OUTPUT_SURFACE;
     }
   }
 
   return capabilities;
 }
 
 }  // namespace media