Merge pull request #25594 from ksooo/pvr-fix-clientid-persist-omega

[Omega][PVR] Fix major design flaw to use std::hash value as persistent client UID

7 files changed, 237 insertions, 26 deletions

diff --git a/xbmc/addons/binary-addons/AddonInstanceHandler.cpp b/xbmc/addons/binary-addons/AddonInstanceHandler.cpp
index 34c303b965..695efd3290 100644
--- a/xbmc/addons/binary-addons/AddonInstanceHandler.cpp
+++ b/xbmc/addons/binary-addons/AddonInstanceHandler.cpp
@@ -79,6 +79,11 @@ std::string IAddonInstanceHandler::ID() const
   return m_addon ? m_addon->ID() : "";
 }
 
+AddonInstanceId IAddonInstanceHandler::InstanceID() const
+{
+  return m_instanceId;
+}
+
 std::string IAddonInstanceHandler::Name() const
 {
   return m_addon ? m_addon->Name() : "";
diff --git a/xbmc/addons/binary-addons/AddonInstanceHandler.h b/xbmc/addons/binary-addons/AddonInstanceHandler.h
index 27ba95b2be..d3b40e03d4 100644
--- a/xbmc/addons/binary-addons/AddonInstanceHandler.h
+++ b/xbmc/addons/binary-addons/AddonInstanceHandler.h
@@ -70,6 +70,7 @@ public:
   const std::string& UniqueWorkID() { return m_uniqueWorkID; }
 
   std::string ID() const;
+  AddonInstanceId InstanceID() const;
   std::string Name() const;
   std::string Author() const;
   std::string Icon() const;
diff --git a/xbmc/pvr/PVRDatabase.cpp b/xbmc/pvr/PVRDatabase.cpp
index a26bd5f636..0dfa051d8b 100644
--- a/xbmc/pvr/PVRDatabase.cpp
+++ b/xbmc/pvr/PVRDatabase.cpp
@@ -9,8 +9,12 @@
 #include "PVRDatabase.h"
 
 #include "ServiceBroker.h"
+#include "addons/AddonManager.h"
+#include "addons/addoninfo/AddonInfo.h"
+#include "addons/addoninfo/AddonType.h"
 #include "dbwrappers/dataset.h"
 #include "pvr/addons/PVRClient.h"
+#include "pvr/addons/PVRClientUID.h"
 #include "pvr/channels/PVRChannel.h"
 #include "pvr/channels/PVRChannelGroupMember.h"
 #include "pvr/channels/PVRChannelGroups.h"
@@ -28,6 +32,7 @@
 #include <memory>
 #include <mutex>
 #include <string>
+#include <tuple>
 #include <utility>
 #include <vector>
 
@@ -156,7 +161,8 @@ void CPVRDatabase::CreateTables()
               "bHasArchive          bool, "
               "iClientProviderUid   integer, "
               "bIsUserSetHidden     bool, "
-              "iLastWatchedGroupId  integer"
+              "iLastWatchedGroupId  integer, "
+              "sDateTimeAdded       varchar(20)"
               ")");
 
   CLog::LogFC(LOGDEBUG, LOGPVR, "Creating table 'channelgroups'");
@@ -189,12 +195,12 @@ void CPVRDatabase::CreateTables()
   );
 
   CLog::LogFC(LOGDEBUG, LOGPVR, "Creating table 'clients'");
-  m_pDS->exec(
-      "CREATE TABLE clients ("
-        "idClient  integer primary key, "
-        "iPriority integer"
-      ")"
-  );
+  m_pDS->exec("CREATE TABLE clients ("
+              "idClient  integer primary key, "
+              "iPriority integer, "
+              "sAddonID TEXT, "
+              "iInstanceID integer"
+              ")");
 
   CLog::LogFC(LOGDEBUG, LOGPVR, "Creating table 'timers'");
   m_pDS->exec(sqlCreateTimersTable);
@@ -368,6 +374,20 @@ void CPVRDatabase::UpdateTables(int iVersion)
     m_pDS->exec("ALTER TABLE channels ADD iLastWatchedGroupId integer");
     m_pDS->exec("UPDATE channels SET iLastWatchedGroupId = -1");
   }
+
+  if (iVersion < 45)
+  {
+    m_pDS->exec("ALTER TABLE channels ADD sDateTimeAdded varchar(20)");
+    m_pDS->exec("UPDATE channels SET sDateTimeAdded = ''");
+  }
+
+  if (iVersion < 46)
+  {
+    m_pDS->exec("ALTER TABLE clients ADD sAddonID TEXT");
+    m_pDS->exec("ALTER TABLE clients ADD iInstanceID integer");
+
+    FixupClientIDs();
+  }
 }
 
 /********** Client methods **********/
@@ -389,10 +409,10 @@ bool CPVRDatabase::Persist(const CPVRClient& client)
 
   std::unique_lock<CCriticalSection> lock(m_critSection);
 
-  const std::string strQuery = PrepareSQL("REPLACE INTO clients (idClient, iPriority) VALUES (%i, %i);",
-                                          client.GetID(), client.GetPriority());
-
-  return ExecuteQuery(strQuery);
+  const std::string sql{PrepareSQL(
+      "REPLACE INTO clients (idClient, iPriority, sAddonID, iInstanceID) VALUES (%i, %i, '%s', %i)",
+      client.GetID(), client.GetPriority(), client.ID().c_str(), client.InstanceID())};
+  return ExecuteQuery(sql);
 }
 
 bool CPVRDatabase::Delete(const CPVRClient& client)
@@ -428,6 +448,113 @@ int CPVRDatabase::GetPriority(const CPVRClient& client) const
   return atoi(strValue.c_str());
 }
 
+void CPVRDatabase::FixupClientIDs()
+{
+  // Get enabled and disabled PVR client addon infos
+  std::vector<ADDON::AddonInfoPtr> addonInfos;
+  CServiceBroker::GetAddonMgr().GetAddonInfos(addonInfos, false, ADDON::AddonType::PVRDLL);
+
+  std::vector<std::tuple<std::string, ADDON::AddonInstanceId, std::string>> clientInfos;
+  for (const auto& addonInfo : addonInfos)
+  {
+    const std::vector<ADDON::AddonInstanceId> instanceIds{addonInfo->GetKnownInstanceIds()};
+    for (const auto instanceId : instanceIds)
+    {
+      clientInfos.emplace_back(addonInfo->ID(), instanceId, addonInfo->Name());
+    }
+  }
+
+  for (const auto& [addonID, instanceID, addonName] : clientInfos)
+  {
+    // Entry with legacy client id present in clients or channels table?
+    const CPVRClientUID uid{addonID, instanceID};
+    int legacyID{uid.GetLegacyUID()};
+    std::string sql{PrepareSQL("idClient = %i", legacyID)};
+    int id{GetSingleValueInt("clients", "idClient", sql)};
+    if (id == legacyID)
+    {
+      // Add addon id and instance id to existing clients table entry.
+      sql = PrepareSQL("UPDATE clients SET sAddonID = '%s', iInstanceID = %i WHERE idClient = %i",
+                       addonID.c_str(), instanceID, legacyID);
+      ExecuteQuery(sql);
+    }
+    else
+    {
+      sql = PrepareSQL("iClientId = %i", legacyID);
+      id = GetSingleValueInt("channels", "iClientId", sql);
+      if (id == legacyID)
+      {
+        // Create a new entry with the legacy client id in clients table.
+        sql = PrepareSQL("REPLACE INTO clients (idClient, iPriority, sAddonID, iInstanceID) VALUES "
+                         "(%i, %i, '%s', %i)",
+                         legacyID, 0, addonID.c_str(), instanceID);
+        ExecuteQuery(sql);
+      }
+      else
+      {
+        // The legacy id was not found in channels table. This happens if the std::hash
+        // implementation changed (for example after a Kodi update), which according to std::hash
+        // documentation can happen: "Hash functions are only required to produce the same result
+        // for the same input within a single execution of a program"
+
+        // We can only fix some of the ids in this case: We can try to find the legacy id via the
+        // addon's name in the providers table. This is not guaranteed to always work (theoretically
+        // addon name might have changed) and cannot work if providers table contains more than one
+        // entry for the same multi-instance addon.
+
+        sql = PrepareSQL("SELECT iClientId FROM providers WHERE iType = 1 AND sName = '%s'",
+                         addonName.c_str());
+
+        if (ResultQuery(sql))
+        {
+          if (m_pDS->num_rows() != 1)
+          {
+            CLog::Log(LOGERROR, "Unable to fixup client id {} for addon '{}', instance {}!",
+                      legacyID, addonID.c_str(), instanceID);
+          }
+          else
+          {
+            // There is exactly one provider with the addon name in question.
+            // Its client id is the legacy id we're looking for!
+            try
+            {
+              legacyID = m_pDS->fv("iClientId").get_asInt();
+              sql = PrepareSQL(
+                  "REPLACE INTO clients (idClient, iPriority, sAddonID, iInstanceID) VALUES "
+                  "(%i, %i, '%s', %i)",
+                  legacyID, 0, addonID.c_str(), instanceID);
+              ExecuteQuery(sql);
+            }
+            catch (...)
+            {
+              CLog::LogF(LOGERROR, "Couldn't obtain providers for addon '{}'.", addonID);
+            }
+          }
+        }
+      }
+    }
+  }
+}
+
+int CPVRDatabase::GetClientID(const std::string& addonID, unsigned int instanceID)
+{
+  std::unique_lock<CCriticalSection> lock(m_critSection);
+
+  // Client id already present in clients table?
+  std::string sql{PrepareSQL("sAddonID = '%s' AND iInstanceID = %i", addonID.c_str(), instanceID)};
+  const std::string idString{GetSingleValue("clients", "idClient", sql)};
+  if (!idString.empty())
+    return std::atoi(idString.c_str());
+
+  // Create a new entry with a new client id in clients table.
+  sql = PrepareSQL("INSERT INTO clients (iPriority, sAddonID, iInstanceID) VALUES (%i, '%s', %i)",
+                   0, addonID.c_str(), instanceID);
+  if (ExecuteQuery(sql))
+    return static_cast<int>(m_pDS->lastinsertid());
+
+  return -1;
+}
+
 /********** Channel provider methods **********/
 
 bool CPVRDatabase::DeleteProviders()
@@ -1037,15 +1164,17 @@ bool CPVRDatabase::Persist(CPVRChannel& channel, bool bCommit)
         "INSERT INTO channels ("
         "iUniqueId, bIsRadio, bIsHidden, bIsUserSetIcon, bIsUserSetName, bIsLocked, "
         "sIconPath, sChannelName, bIsVirtual, bEPGEnabled, sEPGScraper, iLastWatched, iClientId, "
-        "idEpg, bHasArchive, iClientProviderUid, bIsUserSetHidden, iLastWatchedGroupId) "
-        "VALUES (%i, %i, %i, %i, %i, %i, '%s', '%s', %i, %i, '%s', %u, %i, %i, %i, %i, %i, %i)",
+        "idEpg, bHasArchive, iClientProviderUid, bIsUserSetHidden, iLastWatchedGroupId, "
+        "sDateTimeAdded) "
+        "VALUES (%i, %i, %i, %i, %i, %i, '%s', '%s', %i, %i, '%s', %u, %i, %i, %i, %i, %i, %i, "
+        "'%s')",
         channel.UniqueID(), (channel.IsRadio() ? 1 : 0), (channel.IsHidden() ? 1 : 0),
         (channel.IsUserSetIcon() ? 1 : 0), (channel.IsUserSetName() ? 1 : 0),
         (channel.IsLocked() ? 1 : 0), channel.IconPath().c_str(), channel.ChannelName().c_str(), 0,
         (channel.EPGEnabled() ? 1 : 0), channel.EPGScraper().c_str(),
         static_cast<unsigned int>(channel.LastWatched()), channel.ClientID(), channel.EpgID(),
         channel.HasArchive(), channel.ClientProviderUid(), channel.IsUserSetHidden() ? 1 : 0,
-        channel.LastWatchedGroupId());
+        channel.LastWatchedGroupId(), "");
   }
   else
   {
@@ -1054,8 +1183,10 @@ bool CPVRDatabase::Persist(CPVRChannel& channel, bool bCommit)
         "REPLACE INTO channels ("
         "iUniqueId, bIsRadio, bIsHidden, bIsUserSetIcon, bIsUserSetName, bIsLocked, "
         "sIconPath, sChannelName, bIsVirtual, bEPGEnabled, sEPGScraper, iLastWatched, iClientId, "
-        "idChannel, idEpg, bHasArchive, iClientProviderUid, bIsUserSetHidden, iLastWatchedGroupId) "
-        "VALUES (%i, %i, %i, %i, %i, %i, '%s', '%s', %i, %i, '%s', %u, %i, %s, %i, %i, %i, %i, %i)",
+        "idChannel, idEpg, bHasArchive, iClientProviderUid, bIsUserSetHidden, iLastWatchedGroupId, "
+        "sDateTimeAdded) "
+        "VALUES (%i, %i, %i, %i, %i, %i, '%s', '%s', %i, %i, '%s', %u, %i, %s, %i, %i, %i, %i, %i, "
+        "'%s')",
         channel.UniqueID(), (channel.IsRadio() ? 1 : 0), (channel.IsHidden() ? 1 : 0),
         (channel.IsUserSetIcon() ? 1 : 0), (channel.IsUserSetName() ? 1 : 0),
         (channel.IsLocked() ? 1 : 0), channel.ClientIconPath().c_str(),
@@ -1063,7 +1194,7 @@ bool CPVRDatabase::Persist(CPVRChannel& channel, bool bCommit)
         channel.EPGScraper().c_str(), static_cast<unsigned int>(channel.LastWatched()),
         channel.ClientID(), strValue.c_str(), channel.EpgID(), channel.HasArchive(),
         channel.ClientProviderUid(), channel.IsUserSetHidden() ? 1 : 0,
-        channel.LastWatchedGroupId());
+        channel.LastWatchedGroupId(), "");
   }
 
   if (QueueInsertQuery(strQuery))
diff --git a/xbmc/pvr/PVRDatabase.h b/xbmc/pvr/PVRDatabase.h
index 7a8932e95d..69272b5503 100644
--- a/xbmc/pvr/PVRDatabase.h
+++ b/xbmc/pvr/PVRDatabase.h
@@ -64,7 +64,7 @@ namespace PVR
      * @brief Get the minimal database version that is required to operate correctly.
      * @return The minimal database version.
      */
-    int GetSchemaVersion() const override { return 44; }
+    int GetSchemaVersion() const override { return 46; }
 
     /*!
      * @brief Get the default sqlite database filename.
@@ -102,6 +102,14 @@ namespace PVR
      */
     int GetPriority(const CPVRClient& client) const;
 
+    /*!
+     * @brief Get the numeric client ID for given addon ID and instance ID from the database.
+     * @param addonID The addon ID.
+     * @param instanceID The instance ID.
+     * @return The client ID on success, -1 otherwise.
+     */
+    int GetClientID(const std::string& addonID, unsigned int instanceID);
+
     /*! @name Channel methods */
     //@{
 
@@ -327,6 +335,8 @@ namespace PVR
 
     bool RemoveChannelsFromGroup(const CPVRChannelGroup& group);
 
+    void FixupClientIDs();
+
     mutable CCriticalSection m_critSection;
   };
 }
diff --git a/xbmc/pvr/addons/PVRClientUID.cpp b/xbmc/pvr/addons/PVRClientUID.cpp
index 87883851ea..72b5cbed02 100644
--- a/xbmc/pvr/addons/PVRClientUID.cpp
+++ b/xbmc/pvr/addons/PVRClientUID.cpp
@@ -8,26 +8,68 @@
 
 #include "PVRClientUID.h"
 
+#include "pvr/PVRDatabase.h"
+#include "utils/log.h"
+
 #include <functional>
+#include <map>
+#include <utility>
 
 using namespace PVR;
 
+namespace
+{
+using ClientUIDParts = std::pair<std::string, ADDON::AddonInstanceId>;
+static std::map<ClientUIDParts, int> s_idMap;
+} // unnamed namespace
+
 int CPVRClientUID::GetUID() const
 {
   if (!m_uidCreated)
   {
-    std::hash<std::string> hasher;
+    const auto it = s_idMap.find({m_addonID, m_instanceID});
+    if (it != s_idMap.cend())
+    {
+      // Cache hit
+      m_uid = (*it).second;
+    }
+    else
+    {
+      // Cache miss. Read from db and cache.
+      CPVRDatabase db;
+      if (!db.Open())
+      {
+        CLog::LogF(LOGERROR, "Unable to open TV database!");
+        return -1;
+      }
 
-    // Note: For database backwards compatibility reasons the hash of the first instance
-    // must be calculated just from the addonId, not from addonId and instanceId.
-    m_uid = static_cast<int>(hasher(
-        (m_instanceID > ADDON::ADDON_FIRST_INSTANCE_ID ? std::to_string(m_instanceID) + "@" : "") +
-        m_addonID));
-    if (m_uid < 0)
-      m_uid = -m_uid;
+      m_uid = db.GetClientID(m_addonID, m_instanceID);
+      if (m_uid == -1)
+      {
+        CLog::LogF(LOGERROR, "Unable to get client id from TV database!");
+        return -1;
+      }
 
+      s_idMap.insert({{m_addonID, m_instanceID}, m_uid});
+    }
     m_uidCreated = true;
   }
 
   return m_uid;
 }
+
+int CPVRClientUID::GetLegacyUID() const
+{
+  // Note: For database backwards compatibility reasons the hash of the first instance
+  // must be calculated just from the addonId, not from addonId and instanceId.
+  std::string prefix;
+  if (m_instanceID > ADDON::ADDON_FIRST_INSTANCE_ID)
+    prefix = std::to_string(m_instanceID) + "@";
+
+  std::hash<std::string> hasher;
+  int uid{static_cast<int>(hasher(prefix + m_addonID))};
+  if (uid < 0)
+    uid = -uid;
+
+  return uid;
+}
diff --git a/xbmc/pvr/addons/PVRClientUID.h b/xbmc/pvr/addons/PVRClientUID.h
index 5b5d1c0507..d3acec2dcf 100644
--- a/xbmc/pvr/addons/PVRClientUID.h
+++ b/xbmc/pvr/addons/PVRClientUID.h
@@ -30,6 +30,12 @@ public:
    */
   int GetUID() const;
 
+  /*!
+   * @brief Return the numeric legacy UID (compatibility/migration purposes only).
+   * @return The numeric legacy UID.
+   */
+  int GetLegacyUID() const;
+
 private:
   CPVRClientUID() = delete;
 
diff --git a/xbmc/pvr/addons/PVRClients.cpp b/xbmc/pvr/addons/PVRClients.cpp
index b2261d376d..8adf8ba934 100644
--- a/xbmc/pvr/addons/PVRClients.cpp
+++ b/xbmc/pvr/addons/PVRClients.cpp
@@ -126,9 +126,17 @@ void CPVRClients::UpdateClients(
             instanceEnabled = client->IsEnabled();
 
           if (instanceEnabled)
+          {
+            CLog::LogF(LOGINFO, "Creating PVR client: addonId={}, instanceId={}, clientId={}",
+                       addon->ID(), instanceId, clientId);
             clientsToCreate.emplace_back(client);
+          }
           else if (isKnownClient)
+          {
+            CLog::LogF(LOGINFO, "Destroying PVR client: addonId={}, instanceId={}, clientId={}",
+                       addon->ID(), instanceId, clientId);
             clientsToDestroy.emplace_back(clientId);
+          }
         }
         else if (IsCreatedClient(clientId))
         {
@@ -140,9 +148,17 @@ void CPVRClients::UpdateClients(
           }
 
           if (instanceEnabled)
+          {
+            CLog::LogF(LOGINFO, "Recreating PVR client: addonId={}, instanceId={}, clientId={}",
+                       addon->ID(), instanceId, clientId);
             clientsToReCreate.emplace_back(clientId, addon->Name());
+          }
           else
+          {
+            CLog::LogF(LOGINFO, "Destroying PVR client: addonId={}, instanceId={}, clientId={}",
+                       addon->ID(), instanceId, clientId);
             clientsToDestroy.emplace_back(clientId);
+          }
         }
       }
     }