Merge pull request #23098 from garbear/fix-memleak

RetroPlayer: Fix memory exhaustion with zero-copy emulators

4 files changed, 41 insertions, 51 deletions

diff --git a/xbmc/cores/RetroPlayer/rendering/RPRenderManager.cpp b/xbmc/cores/RetroPlayer/rendering/RPRenderManager.cpp
index a0886fe26a..5e22ed887f 100644
--- a/xbmc/cores/RetroPlayer/rendering/RPRenderManager.cpp
+++ b/xbmc/cores/RetroPlayer/rendering/RPRenderManager.cpp
@@ -123,35 +123,56 @@ bool CRPRenderManager::Configure(AVPixelFormat format,
   return true;
 }
 
-std::vector<VideoStreamBuffer> CRPRenderManager::GetVideoBuffers(unsigned int width,
-                                                                 unsigned int height)
+bool CRPRenderManager::GetVideoBuffer(unsigned int width,
+                                      unsigned int height,
+                                      VideoStreamBuffer& buffer)
 {
-  std::vector<VideoStreamBuffer> buffers;
+  // Clear any previous pending buffers
+  for (IRenderBuffer* buffer : m_pendingBuffers)
+    buffer->Release();
+  m_pendingBuffers.clear();
 
   if (m_bFlush || m_state != RENDER_STATE::CONFIGURED)
-    return buffers;
+    return false;
 
-  // Get buffers from visible renderers
-  for (IRenderBufferPool* bufferPool : m_processInfo.GetBufferManager().GetBufferPools())
-  {
-    if (!bufferPool->HasVisibleRenderer())
-      continue;
+  // We should do our best to get a valid render buffer. If we return false,
+  // the game add-on will likely allocate its own memory.
+  IRenderBuffer* renderBuffer = nullptr;
 
-    IRenderBuffer* renderBuffer = bufferPool->GetBuffer(width, height);
+  auto bufferPools = m_processInfo.GetBufferManager().GetBufferPools();
+
+  std::sort(bufferPools.begin(), bufferPools.end(),
+            [](const IRenderBufferPool* lhs, const IRenderBufferPool* rhs) {
+              // Prefer buffer pools with a visible renderer
+              if (lhs->HasVisibleRenderer() && !rhs->HasVisibleRenderer())
+                return true;
+              if (!lhs->HasVisibleRenderer() && rhs->HasVisibleRenderer())
+                return false;
+
+              //! @todo De-prioritize buffer pools with write-only or unaligned memory
+
+              return false;
+            });
+
+  for (IRenderBufferPool* bufferPool : bufferPools)
+  {
+    renderBuffer = bufferPool->GetBuffer(width, height);
     if (renderBuffer != nullptr)
-      m_pendingBuffers.emplace_back(renderBuffer);
+      break;
     else
       CLog::Log(LOGDEBUG, "RetroPlayer[RENDER]: Unable to get video buffer for frame");
   }
 
-  for (IRenderBuffer* renderBuffer : m_pendingBuffers)
-  {
-    buffers.emplace_back(VideoStreamBuffer{
-        renderBuffer->GetFormat(), renderBuffer->GetMemory(), renderBuffer->GetFrameSize(),
-        renderBuffer->GetMemoryAccess(), renderBuffer->GetMemoryAlignment()});
-  }
+  if (renderBuffer == nullptr)
+    return false;
+
+  buffer = VideoStreamBuffer{renderBuffer->GetFormat(), renderBuffer->GetMemory(),
+                             renderBuffer->GetFrameSize(), renderBuffer->GetMemoryAccess(),
+                             renderBuffer->GetMemoryAlignment()};
 
-  return buffers;
+  m_pendingBuffers.emplace_back(std::move(renderBuffer));
+
+  return true;
 }
 
 void CRPRenderManager::AddFrame(const uint8_t* data,
diff --git a/xbmc/cores/RetroPlayer/rendering/RPRenderManager.h b/xbmc/cores/RetroPlayer/rendering/RPRenderManager.h
index 14229efad3..bd1ebb59e5 100644
--- a/xbmc/cores/RetroPlayer/rendering/RPRenderManager.h
+++ b/xbmc/cores/RetroPlayer/rendering/RPRenderManager.h
@@ -93,7 +93,7 @@ public:
                  unsigned int maxWidth,
                  unsigned int maxHeight,
                  float pixelAspectRatio);
-  std::vector<VideoStreamBuffer> GetVideoBuffers(unsigned int width, unsigned int height);
+  bool GetVideoBuffer(unsigned int width, unsigned int height, VideoStreamBuffer& buffer);
   void AddFrame(const uint8_t* data,
                 size_t size,
                 unsigned int width,
diff --git a/xbmc/cores/RetroPlayer/streams/RetroPlayerVideo.cpp b/xbmc/cores/RetroPlayer/streams/RetroPlayerVideo.cpp
index 3c740e9dc8..14c66a86ae 100644
--- a/xbmc/cores/RetroPlayer/streams/RetroPlayerVideo.cpp
+++ b/xbmc/cores/RetroPlayer/streams/RetroPlayerVideo.cpp
@@ -13,8 +13,6 @@
 #include "cores/RetroPlayer/rendering/RenderTranslator.h"
 #include "utils/log.h"
 
-#include <algorithm>
-
 using namespace KODI;
 using namespace RETRO;
 
@@ -74,33 +72,7 @@ bool CRetroPlayerVideo::GetStreamBuffer(unsigned int width,
   VideoStreamBuffer& videoBuffer = static_cast<VideoStreamBuffer&>(buffer);
 
   if (m_bOpen)
-  {
-    m_buffers = m_renderManager.GetVideoBuffers(width, height);
-
-    std::sort(m_buffers.begin(), m_buffers.end(),
-              [](const VideoStreamBuffer& lhs, const VideoStreamBuffer& rhs) {
-                // Prefer read-write over write only
-                if (lhs.access == DataAccess::READ_WRITE && rhs.access != DataAccess::READ_WRITE)
-                  return true;
-                if (lhs.access != DataAccess::READ_WRITE && rhs.access == DataAccess::READ_WRITE)
-                  return false;
-
-                // Prefer aligned over unaligned
-                if (lhs.alignment == DataAlignment::DATA_ALIGNED &&
-                    rhs.alignment != DataAlignment::DATA_ALIGNED)
-                  return true;
-                if (lhs.alignment != DataAlignment::DATA_ALIGNED &&
-                    rhs.alignment == DataAlignment::DATA_ALIGNED)
-                  return false;
-
-                return false;
-              });
-
-    //! @todo This comment was in the original code
-    //! @todo Handle multiple buffers
-    if (!m_buffers.empty())
-      videoBuffer = m_buffers.at(0);
-  }
+    return m_renderManager.GetVideoBuffer(width, height, videoBuffer);
 
   return false;
 }
@@ -130,8 +102,6 @@ void CRetroPlayerVideo::AddStreamData(const StreamPacket& packet)
     m_renderManager.AddFrame(videoPacket.data, videoPacket.size, videoPacket.width,
                              videoPacket.height, orientationDegCCW);
   }
-
-  m_buffers.clear();
 }
 
 void CRetroPlayerVideo::CloseStream()
diff --git a/xbmc/cores/RetroPlayer/streams/RetroPlayerVideo.h b/xbmc/cores/RetroPlayer/streams/RetroPlayerVideo.h
index 06ad6245c8..8d153ab5c2 100644
--- a/xbmc/cores/RetroPlayer/streams/RetroPlayerVideo.h
+++ b/xbmc/cores/RetroPlayer/streams/RetroPlayerVideo.h
@@ -107,7 +107,6 @@ private:
 
   // Stream properties
   bool m_bOpen = false;
-  std::vector<VideoStreamBuffer> m_buffers;
 };
 } // namespace RETRO
 } // namespace KODI