Fixes in device ownership changes

- When RequestConfigurationChange() is invoked before Device
  is attached to object hierarchy (i.e. doesn't have owner),
  apply changes in-place.

  This fixes a bug when calling AddStreamAsync() before
  calling AddDevice() would never create the stream.

- Handle races between RequestConfigurationChange() and changing
  device owner from Plugin (in AddDevice and RemoveDevice).

  To avoid races, a new method Device::RequestOwnershipChange()
  is added, which is serialized with RequestConfigurationChange().

  Plugin now uses that method to change device ownership.

- Allow user to override all 4 methods:
  RequestConfigurationChange(), RequestOwnershipChange(),
  PerformConfigurationChange(), AbortConfigurationChange()

  Useful if the user want to replace handling of request changes
  entirely.

Author: gavv

include/aspl/Device.hpp

@@ -921,14 +921,31 @@ class Device : public Object
     //!  some time later HAL invokes PerformConfigurationChange(), which runs the
     //!  function. HAL may also invoke AbortConfigurationChange() instead.
     //! @note
-    //!  If Context::Host is null, this method assumes that the plugin is not yet
-    //!  initialized and published to HAL, and in this case it just executes the
-    //!  function immediately.
-    //! @note
-    //!  If invoked from PerformConfigurationChange(), assumes that we're already
-    //!  at the point where it's safe to change configuration and executes the
-    //!  function immediately.
-    void RequestConfigurationChange(std::function<void()> func = {});
+    //!  There are three cases when this method decides that it's safe to invoke the
+    //!  function immediately ("in-place") instead of enqueueing it:
+    //!   - If Context::Host is null, the method assumes that the entire plugin is not
+    //!     yet initialized and published to HAL, and thus HAL doesn't use Device.
+    //!   - If Device::HasOwner() is false, the method assumes that the device itself
+    //!     is not added to the plugin hierarchy yet, and thus HAL doesn't use Device.
+    //!   - If the method is invoked from PerformConfigurationChange(), it assumes
+    //!     that we're already at the point where it's safe to change configuration.
+    virtual void RequestConfigurationChange(std::function<void()> func = {});
+
+    //! Request device to change its owner.
+    //! @remarks
+    //!  This method is called by Device owner (Plugin object) when device is added
+    //!  or removed to the plugin (and so becomes published or unpublished from HAL).
+    //!  This method implements thread-safe changing of the ownership and handles
+    //!  possible races with RequestConfigurationChange().
+    //!  It is needed because RequestConfigurationChange() mechanism is tightly
+    //!  coupled with the ownership state and should be taken into account when
+    //!  ownership changes.
+    //! @note
+    //!  Normally @p owner is always a pointer to Plugin, and @p shouldHaveOwnership
+    //!  is true when adding device and false when removing.
+    //!  This method is expected to do all necessary housekeeping and invoke
+    //!  owner->AddOwnedObject(this) or owner->RemoveOwnedObject(this).
+    virtual void RequestOwnershipChange(Object* owner, bool shouldHaveOwnership);
 
     //! Called by the Host to allow the device to perform a configuration change
     //! that had been previously requested via a call to the Host method,
@@ -1246,7 +1263,7 @@ class Device : public Object
     DoubleBuffer<std::variant<std::shared_ptr<IORequestHandler>, IORequestHandler*>>
         ioHandler_;
 
-    std::unordered_map<UInt64, std::function<void()>> pendingConfigurationRequests_;
+    std::map<UInt64, std::function<void()>> pendingConfigurationRequests_;
     UInt64 lastConfigurationRequestID_ = 0;
     UInt64 insideConfigurationHandler_ = 0;
 

include/aspl/Object.hpp

@@ -106,6 +106,14 @@ class Object : public std::enable_shared_from_this<Object>
     //!  Backs @c kAudioObjectPropertyOwner property.
     AudioObjectID GetOwnerID() const;
 
+    //! Check if the object is part of the hierarchy.
+    //! Returns true if GetOwnerID() is not equal to kAudioObjectUnknown.
+    //! @remarks
+    //!  By default, object does not have an owner. Until the object is
+    //!  attached to an owner, it is not part of the plugin object
+    //!  hierarchy and is not visible to HAL.
+    bool HasOwner() const;
+
     //! Get owned objects.
     //! Returns the list of objects to which this object is the owner.
     //! @remarks

include/aspl/Plugin.hpp

@@ -164,7 +164,7 @@ class Plugin : public Object
     //! @}
 
 private:
-    mutable std::mutex writeMutex_;
+    mutable std::recursive_mutex writeMutex_;
 
     const PluginParameters params_;
 

src/Device.cpp

@@ -1592,7 +1592,11 @@ void Device::RequestConfigurationChange(std::function<void()> func)
 
     auto host = GetContext()->Host.load();
 
-    if (host && !insideConfigurationHandler_) {
+    // Note: RequestConfigurationChange(), RequestOwnershipChange(), and
+    // PerformConfigurationChange() calls are serialized via a mutex.
+    // RequestOwnershipChange() changes HasOwner(), and PerformConfigurationChange()
+    // changes insideConfigurationHandler_.
+    if (host && HasOwner() && !insideConfigurationHandler_) {
         const auto reqID = lastConfigurationRequestID_++;
 
         GetContext()->Tracer->Message(
@@ -1610,6 +1614,68 @@ void Device::RequestConfigurationChange(std::function<void()> func)
     }
 }
 
+void Device::RequestOwnershipChange(Object* owner, bool shouldHaveOwnership)
+{
+    std::lock_guard writeLock(writeMutex_);
+
+    if (shouldHaveOwnership) {
+        if (HasOwner()) {
+            // Device owner is always either empty or Plugin object.
+            // If we already have an owner, we assume that it's
+            // already set to the same object and do nothing.
+            return;
+        }
+
+        GetContext()->Tracer->Message(
+            "Device::RequestOwnershipChange() attaching owner devID=%lu ownerID=%lu",
+            static_cast<unsigned long>(GetID()),
+            static_cast<unsigned long>(owner->GetID()));
+
+        // Device becomes visible to HAL.
+        // HasOwner() now will return true.
+        // RequestConfigurationChange() will enqueue changes instead of applying in-place.
+        owner->AddOwnedObject(shared_from_this());
+    } else {
+        if (!HasOwner()) {
+            // If we already don't have an owner, do nothing.
+            return;
+        }
+
+        GetContext()->Tracer->Message(
+            "Device::RequestOwnershipChange() detaching owner devID=%lu ownerID=%lu",
+            static_cast<unsigned long>(GetID()),
+            static_cast<unsigned long>(owner->GetID()));
+
+        // Device disappears HAL.
+        // HasOwner() now will return false.
+        // RequestConfigurationChange() will apply changes in-place.
+        owner->RemoveOwnedObject(GetID());
+
+        if (!pendingConfigurationRequests_.empty()) {
+            // If there are any enqueued changes, apply them now since it's not
+            // guaranteed now that HAL will ever handle them. But HAL *will* try
+            // to apply (some of) them, PerformConfigurationChange() will just
+            // ignore those requests because we've removed them from map here.
+            GetContext()->Tracer->Message(
+                "Device::RequestOwnershipChange()"
+                " applying pending configuration changes in-place"
+                " devID=%lu  numChanges=%lu",
+                static_cast<unsigned long>(GetID()),
+                static_cast<unsigned long>(pendingConfigurationRequests_.size()));
+
+            // Iterate ordered map from lower to higher request IDs.
+            while (!pendingConfigurationRequests_.empty()) {
+                auto it = pendingConfigurationRequests_.begin();
+
+                const auto& func = it->second;
+                func();
+
+                pendingConfigurationRequests_.erase(it);
+            }
+        }
+    }
+}
+
 OSStatus Device::PerformConfigurationChange(AudioObjectID objectID,
     UInt64 changeAction,
     void* changeInfo)
@@ -1630,7 +1696,7 @@ OSStatus Device::PerformConfigurationChange(AudioObjectID objectID,
         func();
     } else {
         GetContext()->Tracer->Message(
-            "Device::PerformConfigurationChange() request func is null reqID=%lu",
+            "Device::PerformConfigurationChange() ignoring null change request reqID=%lu",
             static_cast<unsigned long>(reqID));
     }
 

src/Object.cpp

@@ -77,6 +77,11 @@ AudioObjectID Object::GetOwnerID() const
     return ownerObjectID_;
 }
 
+bool Object::HasOwner() const
+{
+    return ownerObjectID_ != kAudioObjectUnknown;
+}
+
 std::vector<AudioObjectID> Object::GetOwnedObjectIDs(AudioObjectPropertyScope scope,
     AudioClassID classID) const
 {

src/Plugin.cpp

@@ -88,8 +88,6 @@ void Plugin::AddDevice(std::shared_ptr<Device> device)
 
     GetContext()->Tracer->OperationBegin(op);
 
-    AddOwnedObject(device);
-
     {
         auto devices = devices_.Get();
 
@@ -114,6 +112,8 @@ void Plugin::AddDevice(std::shared_ptr<Device> device)
         deviceByUID_.Set(std::move(deviceByUID));
     }
 
+    device->RequestOwnershipChange(this, true);
+
     NotifyPropertiesChanged(
         {kAudioObjectPropertyOwnedObjects, kAudioPlugInPropertyDeviceList});
 
@@ -158,7 +158,7 @@ void Plugin::RemoveDevice(std::shared_ptr<Device> device)
         deviceByUID_.Set(std::move(deviceByUID));
     }
 
-    RemoveOwnedObject(device->GetID());
+    device->RequestOwnershipChange(this, false);
 
     NotifyPropertiesChanged(
         {kAudioObjectPropertyOwnedObjects, kAudioPlugInPropertyDeviceList});