Improve android MediaDrm property latency

[ Merge of http://go/wvgerrit/89848 ]

Apps query a number of properties at initialization. The mediaDrm
API getProperty allows the query of a single property at a time.
This causes a series of requests. If no crypto
sessions are concurrently open, a series of expensive OEMCrypto
Initialization and Termination calls will occur.

In this change OEMCrypto termination is delayed. If an OEMCrypto
Terminate is followed in close succession by an Initialize, neither
will occur avoiding the overhead. A timer enables a countdown process.
If no session activity occurs, the timer will eventually terminate
OEMCrypto and exit.

Bug: 136282358
Test: Android unit/integration tests
Change-Id: I442b7919b4e7835c52583516c8bc64d0c150241d

libwvdrmengine/cdm/core/include/crypto_session.h

@@ -44,11 +44,11 @@ class CryptoSessionFactory;
 class CryptoSession {
  public:
   using HdcpCapability = OEMCrypto_HDCP_Capability;
-  typedef enum {
+  enum UsageDurationStatus {
     kUsageDurationsInvalid = 0,
     kUsageDurationPlaybackNotBegun = 1,
     kUsageDurationsValid = 2,
-  } UsageDurationStatus;
+  };
 
   struct SupportedCertificateTypes {
     bool rsa_2048_bit;
@@ -64,6 +64,17 @@ class CryptoSession {
 
   virtual ~CryptoSession();
 
+  // This method will try to terminate OEMCrypto if |session_size_| is 0.
+  // A platform configured property |delay_oem_crypto_termination| will
+  // determine if termination occurs immediately or after a delay.
+  // If termination is delayed, a countdown mechanism is employed.
+  // Call |TryTerminate| periodically until it no longer returns true.
+  // To immediately terminate call |DisableDelayedTermination| before calling
+  // |TryTerminate|.
+  static bool TryTerminate();
+
+  static void DisableDelayedTermination();
+
   virtual CdmResponseType GetProvisioningToken(std::string* client_token);
   virtual CdmClientTokenType GetPreProvisionTokenType() {
     return pre_provision_token_type_;
@@ -282,7 +293,6 @@ class CryptoSession {
   }
 
   void Init();
-  void Terminate();
   CdmResponseType GetTokenFromKeybox(std::string* token);
   CdmResponseType GetTokenFromOemCert(std::string* token);
   static bool ExtractSystemIdFromOemCert(const std::string& oem_cert,
@@ -385,6 +395,7 @@ class CryptoSession {
 
   static bool initialized_;
   static int session_count_;
+  static int termination_counter_;
 
   metrics::CryptoMetrics* metrics_;
   metrics::TimerMetric life_span_;

libwvdrmengine/cdm/core/src/crypto_session.cpp

@@ -60,6 +60,7 @@ const size_t kOemCryptoApiVersionSupportsSwitchingCipherMode = 14;
 
 // Constants and utility objects relating to OEM Certificates
 const char* const kWidevineSystemIdExtensionOid = "1.3.6.1.4.1.11129.4.1.1";
+constexpr int kMaxTerminateCountDown = 5;
 
 const std::string kStringNotAvailable = "NA";
 
@@ -96,6 +97,7 @@ shared_mutex CryptoSession::static_field_mutex_;
 shared_mutex CryptoSession::oem_crypto_mutex_;
 bool CryptoSession::initialized_ = false;
 int CryptoSession::session_count_ = 0;
+int CryptoSession::termination_counter_ = 0;
 UsageTableHeader* CryptoSession::usage_table_header_l1_ = nullptr;
 UsageTableHeader* CryptoSession::usage_table_header_l3_ = nullptr;
 std::atomic<uint64_t> CryptoSession::request_id_index_source_(0);
@@ -189,7 +191,15 @@ CryptoSession::~CryptoSession() {
   if (open_) {
     Close();
   }
-  Terminate();
+  WithStaticFieldWriteLock("~CryptoSession", [&] {
+    if (session_count_ > 0) {
+      --session_count_;
+    } else {
+      LOGE("Invalid crypto session count: session_count_ = %d", session_count_);
+    }
+  });
+
+  TryTerminate();
   M_RECORD(metrics_, crypto_session_life_span_, life_span_.AsMs());
 }
 
@@ -244,6 +254,9 @@ void CryptoSession::Init() {
         LOGE("OEMCrypto_Initialize failed: status = %d", static_cast<int>(sts));
         return;
       }
+      termination_counter_ = Properties::delay_oem_crypto_termination()
+                                 ? kMaxTerminateCountDown
+                                 : 0;
       initialized_ = true;
       initialized = true;
     }
@@ -273,17 +286,20 @@ void CryptoSession::Init() {
   }
 }
 
-void CryptoSession::Terminate() {
+bool CryptoSession::TryTerminate() {
   LOGV("Terminating crypto session");
-  WithStaticFieldWriteLock("Terminate", [&] {
+  WithStaticFieldWriteLock("TryTerminate", [&] {
-    LOGV("Terminating crypto session: initialized_ = %s, session_count_ = %d",
+    LOGV(
-         initialized_ ? "true" : "false", session_count_);
+        "Terminating crypto session: initialized_ = %s, session_count_ = %d, "
-    if (session_count_ > 0) {
+        "termination_counter_ = %d",
-      session_count_ -= 1;
+        initialized_ ? "true" : "false", session_count_, termination_counter_);
-    } else {
+
-      LOGE("Invalid crypto session count: session_count_ = %d", session_count_);
+    if (termination_counter_ > 0) {
+      --termination_counter_;
     }
-    if (session_count_ > 0 || !initialized_) return;
+    if (session_count_ > 0 || termination_counter_ > 0 || !initialized_)
+      return false;
+
     OEMCryptoResult sts;
     WithOecWriteLock("Terminate", [&] { sts = OEMCrypto_Terminate(); });
     if (OEMCrypto_SUCCESS != sts) {
@@ -300,7 +316,15 @@ void CryptoSession::Terminate() {
     }
 
     initialized_ = false;
+    return true;
   });
+  return true;
+}
+
+void CryptoSession::DisableDelayedTermination() {
+  LOGV("Disable delayed termination");
+  WithStaticFieldWriteLock("DisableDelayedTermination",
+                           [&] { termination_counter_ = 0; });
 }
 
 CdmResponseType CryptoSession::GetTokenFromKeybox(std::string* token) {
@@ -687,6 +711,11 @@ CdmResponseType CryptoSession::Open(SecurityLevel requested_security_level) {
     return MapOEMCryptoResult(sts, OPEN_CRYPTO_SESSION_ERROR, "Open");
   }
 
+  WithStaticFieldWriteLock("Open() termination_counter", [&] {
+    termination_counter_ =
+        Properties::delay_oem_crypto_termination() ? kMaxTerminateCountDown : 0;
+  });
+
   oec_session_id_ = static_cast<CryptoSessionId>(sid);
   LOGV("Opened session: id = %u", oec_session_id_);
   open_ = true;

libwvdrmengine/cdm/include/timer.h

@@ -43,7 +43,10 @@ class Timer {
   ~Timer();
 
   bool Start(TimerHandler* handler, uint32_t time_in_secs);
-  void Stop();
+  // Enable |wait_for_exit| only if the method is being invoked from a thread
+  // other than the timer thread. Deadlock might occur if enabled and called
+  // from the timer thread.
+  void Stop(bool wait_for_exit);
   bool IsRunning();
 
  private:

libwvdrmengine/cdm/include/wv_content_decryption_module.h

@@ -184,14 +184,17 @@ class WvContentDecryptionModule : public android::RefBase, public TimerHandler {
 
   uint32_t GenerateSessionSharingId();
 
-  // timer related methods to drive policy decisions
+  // Timer related methods to drive policy decisions
-  void EnablePolicyTimer();
+  void EnableTimer();
-  void DisablePolicyTimer();
+  void DisableTimer();
+  // Call this method only if invoked from a thread other than the timer thread.
+  // Otherwise this might result in deadlock.
+  void DisableTimerAndWaitForExit();
   void OnTimerEvent();
 
   static std::mutex session_sharing_id_generation_lock_;
-  std::mutex policy_timer_lock_;
+  std::mutex timer_lock_;
-  Timer policy_timer_;
+  Timer timer_;
 
   // instance variables
   // This manages the lifetime of the CDM instances.

libwvdrmengine/cdm/src/timer.cpp

@@ -28,12 +28,13 @@ class Timer::Impl : virtual public android::RefBase {
       return run("wvcdm::Timer::Impl") == android::NO_ERROR;
     }
 
-    void Stop() {
+    void Stop(bool wait_for_exit) {
-      {
-        android::Mutex::Autolock autoLock(lock_);
       stop_condition_.signal();
-      }
+      if (wait_for_exit) {
         requestExitAndWait();
+      } else {
+        requestExit();
+      }
     }
 
    private:
@@ -63,8 +64,8 @@ class Timer::Impl : virtual public android::RefBase {
     return impl_thread_->Start(handler, time_in_secs);
   }
 
-  void Stop() {
+  void Stop(bool wait_for_exit) {
-    impl_thread_->Stop();
+    impl_thread_->Stop(wait_for_exit);
     impl_thread_.clear();
   }
 
@@ -78,7 +79,7 @@ class Timer::Impl : virtual public android::RefBase {
 Timer::Timer() : impl_(new Timer::Impl()) {}
 
 Timer::~Timer() {
-  if (IsRunning()) Stop();
+  if (IsRunning()) Stop(false);
 }
 
 bool Timer::Start(TimerHandler* handler, uint32_t time_in_secs) {
@@ -87,7 +88,7 @@ bool Timer::Start(TimerHandler* handler, uint32_t time_in_secs) {
   return impl_->Start(handler, time_in_secs);
 }
 
-void Timer::Stop() { impl_->Stop(); }
+void Timer::Stop(bool wait_for_exit) { impl_->Stop(wait_for_exit); }
 
 bool Timer::IsRunning() { return impl_->IsRunning(); }
 

libwvdrmengine/cdm/src/wv_content_decryption_module.cpp

@@ -18,7 +18,7 @@
 #include "wv_cdm_event_listener.h"
 
 namespace {
-const int kCdmPolicyTimerDurationSeconds = 1;
+const int kCdmTimerDurationSeconds = 1;
 }
 
 namespace wvcdm {
@@ -28,8 +28,10 @@ std::mutex WvContentDecryptionModule::session_sharing_id_generation_lock_;
 WvContentDecryptionModule::WvContentDecryptionModule() {}
 
 WvContentDecryptionModule::~WvContentDecryptionModule() {
+  CryptoSession::DisableDelayedTermination();
   CloseAllCdms();
-  DisablePolicyTimer();
+  CryptoSession::TryTerminate();
+  DisableTimerAndWaitForExit();
 }
 
 bool WvContentDecryptionModule::IsSupported(const std::string& init_data_type) {
@@ -123,17 +125,10 @@ CdmResponseType WvContentDecryptionModule::GenerateKeyRequest(
   sts = cdm_engine->GenerateKeyRequest(session_id, key_set_id,
                                        initialization_data, license_type,
                                        app_parameters, key_request);
-  switch (license_type) {
+  if (license_type == kLicenseTypeRelease && sts != KEY_MESSAGE) {
-    case kLicenseTypeRelease:
-      if (sts != KEY_MESSAGE) {
     cdm_engine->CloseKeySetSession(key_set_id);
     cdm_by_session_id_.erase(key_set_id);
   }
-      break;
-    default:
-      if (sts == KEY_MESSAGE) EnablePolicyTimer();
-      break;
-  }
   return sts;
 }
 
@@ -165,7 +160,6 @@ CdmResponseType WvContentDecryptionModule::RestoreKey(
   if (!cdm_engine) return SESSION_NOT_FOUND_4;
   CdmResponseType sts;
   sts = cdm_engine->RestoreKey(session_id, key_set_id);
-  if (sts == KEY_ADDED) EnablePolicyTimer();
   return sts;
 }
 
@@ -355,7 +349,11 @@ WvContentDecryptionModule::CdmInfo::CdmInfo()
 
 CdmEngine* WvContentDecryptionModule::EnsureCdmForIdentifier(
     const CdmIdentifier& identifier) {
+  CdmEngine* cdm_engine;
+  bool enable_timer = false;
+  {
     std::unique_lock<std::mutex> auto_lock(cdms_lock_);
+    if (cdms_.size() == 0) enable_timer = true;
     if (cdms_.find(identifier) == cdms_.end()) {
       // Accessing the map entry will create a new instance using the default
       // constructor. We then need to provide it with two pieces of info: The
@@ -369,7 +367,10 @@ CdmEngine* WvContentDecryptionModule::EnsureCdmForIdentifier(
           identifier.app_package_name);
       cdms_[identifier].cdm_engine->SetSpoid(identifier.spoid);
     }
-  CdmEngine* cdm_engine = cdms_[identifier].cdm_engine.get();
+    cdm_engine = cdms_[identifier].cdm_engine.get();
+  }
+  // Do not enable timer while holding on to the |cdms_lock_|
+  if (enable_timer) EnableTimer();
 
   return cdm_engine;
 }
@@ -392,12 +393,6 @@ void WvContentDecryptionModule::CloseAllCdms() {
 
 CdmResponseType WvContentDecryptionModule::CloseCdm(
     const CdmIdentifier& cdm_identifier) {
-  // The policy timer ultimately calls OnTimerEvent (which wants to
-  // acquire cdms_lock_). Therefore, we cannot acquire cdms_lock_ and then the
-  // policy_timer_lock_ (via DisablePolicyTimer) at the same time.
-  // Acquire the cdms_lock_ first, in its own scope.
-  bool cdms_empty = false;
-  {
   std::unique_lock<std::mutex> auto_lock(cdms_lock_);
   auto it = cdms_.find(cdm_identifier);
   if (it == cdms_.end()) {
@@ -415,12 +410,6 @@ CdmResponseType WvContentDecryptionModule::CloseCdm(
     }
   }
   cdms_.erase(it);
-    cdms_empty = cdms_.empty();
-  }
-
-  if (cdms_empty) {
-    DisablePolicyTimer();
-  }
   return NO_ERROR;
 }
 
@@ -462,24 +451,61 @@ CdmResponseType WvContentDecryptionModule::GetDecryptHashError(
   return cdm_engine->GetDecryptHashError(session_id, hash_error_string);
 }
 
-void WvContentDecryptionModule::EnablePolicyTimer() {
+void WvContentDecryptionModule::EnableTimer() {
-  std::unique_lock<std::mutex> auto_lock(policy_timer_lock_);
+  std::unique_lock<std::mutex> auto_lock(timer_lock_);
-  if (!policy_timer_.IsRunning())
+  if (!timer_.IsRunning()) timer_.Start(this, kCdmTimerDurationSeconds);
-    policy_timer_.Start(this, kCdmPolicyTimerDurationSeconds);
 }
 
-void WvContentDecryptionModule::DisablePolicyTimer() {
+void WvContentDecryptionModule::DisableTimer() {
-  std::unique_lock<std::mutex> auto_lock(policy_timer_lock_);
+  std::unique_lock<std::mutex> auto_lock(timer_lock_);
-  if (policy_timer_.IsRunning()) {
+  if (timer_.IsRunning()) {
-    policy_timer_.Stop();
+    timer_.Stop(false);
+  }
+}
+
+void WvContentDecryptionModule::DisableTimerAndWaitForExit() {
+  std::unique_lock<std::mutex> auto_lock(timer_lock_);
+  if (timer_.IsRunning()) {
+    timer_.Stop(true);
   }
 }
 
 void WvContentDecryptionModule::OnTimerEvent() {
+  bool disable_timer = false;
+  {
     std::unique_lock<std::mutex> auto_lock(cdms_lock_);
     for (auto it = cdms_.begin(); it != cdms_.end(); ++it) {
       it->second.cdm_engine->OnTimerEvent();
     }
+    if (cdms_.empty()) {
+      if (CryptoSession::TryTerminate()) {
+        // If CryptoSession is in a state to be terminated, try acquiring the
+        // |timer_lock_| before deciding whether to disable the timer. If the
+        // lock cannot be acquired, there is no need to disable the timer.
+        // The |timer_lock_| is either being held by
+        // * EnableTimer - This does not appear possible, but if it did
+        //   happen |OnTimerEvent| will be called again. The timer can be
+        //   disabled during a future call.
+        // * DisableTimer - If so, allow that call to disable the timer. No
+        //   need to call to disable it here.
+        // * DisableTimerAndWaitForExit - If so, allow that call to disable
+        //   the timer. No need to call to disable it here. Note that
+        //   |DisableTimerAndWaitForExit| will try to stop the timer but
+        //   wait for it to exit. This might kick off the timer thread one
+        //   last time, which will call into OnTimerEvent. Calling
+        //   |DisableTimer| here will result in deadlock.
+        if (timer_lock_.try_lock()) {
+          disable_timer = true;
+          timer_lock_.unlock();
+        }
+      }
+    }
+  }
+
+  // Release |cdms_lock_| before attempting to disable the timer
+  if (disable_timer) {
+    DisableTimer();
+  }
 }
 
 uint32_t WvContentDecryptionModule::GenerateSessionSharingId() {

libwvdrmengine/cdm/test/timer_unittest.cpp

@@ -8,17 +8,46 @@
 
 namespace wvcdm {
 
+namespace {
+
+// duration in seconds after starting the timer
+constexpr uint32_t kTestDuration = 5;
+// duration in seconds after test completes, before evaluating the result
+constexpr uint32_t kPostTestDuration = 3;
+
+}  // namespace
+
 class TestTimerHandler : public TimerHandler {
  public:
-  TestTimerHandler() : timer_events_(0){};
+  TestTimerHandler() : timer_events_count_(0){};
   virtual ~TestTimerHandler(){};
 
-  virtual void OnTimerEvent() { timer_events_++; }
+  virtual void OnTimerEvent() { timer_events_count_++; }
 
-  uint32_t timer_events() { return timer_events_; }
+  uint32_t timer_events_count() { return timer_events_count_; }
 
- private:
+ protected:
-  uint32_t timer_events_;
+  uint32_t timer_events_count_;
+};
+
+class TestStopTimerHandler : public TestTimerHandler {
+ public:
+  TestStopTimerHandler(uint32_t stop_at_timer_events_count, Timer* timer)
+      : stop_at_timer_events_count_(stop_at_timer_events_count),
+        timer_(timer) {}
+  virtual ~TestStopTimerHandler() {}
+
+  virtual void OnTimerEvent() {
+    if (++timer_events_count_ >= stop_at_timer_events_count_) {
+      if (timer_ != nullptr) {
+        timer_->Stop(false);
+      }
+    }
+  }
+
+ protected:
+  uint32_t stop_at_timer_events_count_;
+  Timer* timer_;
 };
 
 TEST(TimerTest, ParametersCheck) {
@@ -29,25 +58,58 @@ TEST(TimerTest, ParametersCheck) {
   EXPECT_FALSE(timer.Start(&handler, 0));
 }
 
-TEST(TimerTest, TimerCheck) {
+TEST(TimerTest, TimerCheck_StopAndWaitForExit) {
   TestTimerHandler handler;
   Timer timer;
-  uint32_t duration = 10;
 
-  EXPECT_EQ(0u, handler.timer_events());
+  EXPECT_EQ(0u, handler.timer_events_count());
   EXPECT_FALSE(timer.IsRunning());
 
   EXPECT_TRUE(timer.Start(&handler, 1));
   EXPECT_TRUE(timer.IsRunning());
-  sleep(duration);
+  sleep(kTestDuration);
 
-  EXPECT_LE(duration - 1, handler.timer_events());
+  EXPECT_LE(kTestDuration - 1, handler.timer_events_count());
-  EXPECT_LE(handler.timer_events(), duration + 1);
+  EXPECT_LE(handler.timer_events_count(), kTestDuration + 1);
-  timer.Stop();
+  timer.Stop(true);
   EXPECT_FALSE(timer.IsRunning());
-  sleep(duration);
+  sleep(kPostTestDuration);
 
-  EXPECT_LE(duration - 1, handler.timer_events());
+  EXPECT_LE(kTestDuration - 1, handler.timer_events_count());
-  EXPECT_LE(handler.timer_events(), duration + 1);
+  EXPECT_LE(handler.timer_events_count(), kTestDuration + 1);
 }
+
+TEST(TimerTest, TimerCheck_Stop) {
+  TestTimerHandler handler;
+  Timer timer;
+
+  EXPECT_EQ(0u, handler.timer_events_count());
+  EXPECT_FALSE(timer.IsRunning());
+
+  EXPECT_TRUE(timer.Start(&handler, 1));
+  EXPECT_TRUE(timer.IsRunning());
+  sleep(kTestDuration);
+
+  EXPECT_LE(kTestDuration - 1, handler.timer_events_count());
+  EXPECT_LE(handler.timer_events_count(), kTestDuration + 1);
+  timer.Stop(false);
+  sleep(kPostTestDuration);
+
+  EXPECT_FALSE(timer.IsRunning());
+}
+
+TEST(TimerTest, StopOnTimerThread) {
+  Timer timer;
+  TestStopTimerHandler handler(kTestDuration - 2, &timer);
+
+  EXPECT_EQ(0u, handler.timer_events_count());
+  EXPECT_FALSE(timer.IsRunning());
+
+  EXPECT_TRUE(timer.Start(&handler, 1));
+  EXPECT_TRUE(timer.IsRunning());
+  sleep(kTestDuration + kPostTestDuration);
+
+  EXPECT_FALSE(timer.IsRunning());
+}
+
 }  // namespace wvcdm