Merge pull request xbmc#25668 from CrystalP/fix-xaudio2-drain-deiniti…

…alize

[AE][XAudio2] Fix Drain and Deinitialize

xbmc/cores/AudioEngine/Sinks/AESinkXAudio.cpp

@@ -163,12 +163,26 @@ void CAESinkXAudio::Deinitialize()
     {
       m_sourceVoice->Stop();
       m_sourceVoice->FlushSourceBuffers();
+
+      // Stop and FlushSourceBuffers are async, wait for queued buffers to be released by XAudio2.
+      // callbacks don't seem to be called otherwise, with memory leakage.
+      XAUDIO2_VOICE_STATE state{};
+      do
+      {
+        if (WAIT_OBJECT_0 != WaitForSingleObject(m_voiceCallback.mBufferEnd.get(), 500))
+        {
+          CLog::LogF(LOGERROR, "timeout waiting for buffer flush - possible buffer memory leak");
+          break;
+        }
+        m_sourceVoice->GetState(&state, 0);
+      } while (state.BuffersQueued > 0);
+
       m_sinkFrames = 0;
       m_framesInBuffers = 0;
     }
     catch (...)
     {
-      CLog::Log(LOGDEBUG, "{}: Invalidated source voice - Releasing", __FUNCTION__);
+      CLog::Log(LOGERROR, "{}: Invalidated source voice - Releasing", __FUNCTION__);
     }
   }
   m_running = false;
@@ -239,18 +253,8 @@ unsigned int CAESinkXAudio::AddPackets(uint8_t **data, unsigned int frames, unsi
   LARGE_INTEGER timerStop;
   LARGE_INTEGER timerFreq;
 #endif
-  size_t dataLenght = frames * m_format.m_frameSize;
-
-  struct buffer_ctx *ctx = new buffer_ctx;
-  ctx->data = new uint8_t[dataLenght];
-  ctx->frames = frames;
-  ctx->sink = this;
-  memcpy(ctx->data, data[0] + offset * m_format.m_frameSize, dataLenght);
 
-  XAUDIO2_BUFFER xbuffer = {};
-  xbuffer.AudioBytes = dataLenght;
-  xbuffer.pAudioData = ctx->data;
-  xbuffer.pContext = ctx;
+  XAUDIO2_BUFFER xbuffer = BuildXAudio2Buffer(data, frames, offset);
 
   if (!m_running) //first time called, pre-fill buffer then start voice
   {
@@ -259,15 +263,15 @@ unsigned int CAESinkXAudio::AddPackets(uint8_t **data, unsigned int frames, unsi
     if (FAILED(hr))
     {
       CLog::LogF(LOGERROR, "voice submit buffer failed due to {}", WASAPIErrToStr(hr));
-      delete ctx;
+      delete xbuffer.pContext;
       return 0;
     }
     hr = m_sourceVoice->Start(0, XAUDIO2_COMMIT_NOW);
     if (FAILED(hr))
     {
       CLog::LogF(LOGERROR, "voice start failed due to {}", WASAPIErrToStr(hr));
       m_isDirty = true; //flag new device or re-init needed
-      delete ctx;
+      delete xbuffer.pContext;
       return INT_MAX;
     }
     m_sinkFrames += frames;
@@ -292,7 +296,7 @@ unsigned int CAESinkXAudio::AddPackets(uint8_t **data, unsigned int frames, unsi
     if (eventAudioCallback != WAIT_OBJECT_0)
     {
       CLog::LogF(LOGERROR, "voice buffer timed out");
-      delete ctx;
+      delete xbuffer.pContext;
       return INT_MAX;
     }
   }
@@ -319,7 +323,7 @@ unsigned int CAESinkXAudio::AddPackets(uint8_t **data, unsigned int frames, unsi
     #ifdef _DEBUG
     CLog::LogF(LOGERROR, "submiting buffer failed due to {}", WASAPIErrToStr(hr));
 #endif
-    delete ctx;
+    delete xbuffer.pContext;
     return INT_MAX;
   }
 
@@ -843,23 +847,49 @@ void CAESinkXAudio::Drain()
   if(!m_sourceVoice)
     return;
 
-  AEDelayStatus status;
-  GetDelay(status);
-
-  KODI::TIME::Sleep(std::chrono::milliseconds(static_cast<int>(status.GetDelay() * 500)));
-
   if (m_running)
   {
     try
     {
+      // Contrary to MS doc, the voice must play a buffer with end of stream flag for the voice
+      // SamplesPlayed counter to be reset.
+      // Per MS doc, Discontinuity() may not take effect until after the entire buffer queue is
+      // consumed, which wouldn't invoke the callback or reset the voice stats.
+      // Solution: submit a 1 sample buffer with end of stream flag and wait for StreamEnd callback
+      // The StreamEnd event is manual reset so that it cannot be missed even if raised before this
+      // code starts waiting for it
+
+      AddEndOfStreamPacket();
+
+      constexpr uint32_t waitSafety = 100; // extra ms wait in case of scheduler issue
+      DWORD waitRc =
+          WaitForSingleObject(m_voiceCallback.m_StreamEndEvent,
+                              m_framesInBuffers * 1000 / m_format.m_sampleRate + waitSafety);
+
+      if (waitRc != WAIT_OBJECT_0)
+      {
+        if (waitRc == WAIT_FAILED)
+        {
+          //! @todo use FormatMessage for a human readable error message
+          CLog::LogF(LOGERROR,
+                     "error WAIT_FAILED while waiting for queued buffers to drain. GetLastError:{}",
+                     GetLastError());
+        }
+        else
+        {
+          CLog::LogF(LOGERROR, "error {} while waiting for queued buffers to drain.", waitRc);
+        }
+      }
+
       m_sourceVoice->Stop();
-      m_sourceVoice->FlushSourceBuffers();
+      ResetEvent(m_voiceCallback.m_StreamEndEvent);
+
       m_sinkFrames = 0;
       m_framesInBuffers = 0;
     }
     catch (...)
     {
-      CLog::Log(LOGDEBUG, "{}: Invalidated source voice - Releasing", __FUNCTION__);
+      CLog::Log(LOGERROR, "{}: Invalidated source voice - Releasing", __FUNCTION__);
     }
   }
   m_running = false;
@@ -887,3 +917,48 @@ bool CAESinkXAudio::IsUSBDevice()
 #endif
   return false;
 }
+
+bool CAESinkXAudio::AddEndOfStreamPacket()
+{
+  constexpr unsigned int frames{1};
+
+  XAUDIO2_BUFFER xbuffer = BuildXAudio2Buffer(nullptr, frames, 0);
+  xbuffer.Flags = XAUDIO2_END_OF_STREAM;
+
+  HRESULT hr = m_sourceVoice->SubmitSourceBuffer(&xbuffer);
+
+  if (hr != S_OK)
+  {
+    CLog::LogF(LOGERROR, "SubmitSourceBuffer failed due to {:X}", hr);
+    delete xbuffer.pContext;
+    return false;
+  }
+
+  m_sinkFrames += frames;
+  m_framesInBuffers += frames;
+  return true;
+}
+
+XAUDIO2_BUFFER CAESinkXAudio::BuildXAudio2Buffer(uint8_t** data,
+                                                 unsigned int frames,
+                                                 unsigned int offset)
+{
+  const unsigned int dataLength{frames * m_format.m_frameSize};
+
+  struct buffer_ctx* ctx = new buffer_ctx;
+  ctx->data = new uint8_t[dataLength];
+  ctx->frames = frames;
+  ctx->sink = this;
+
+  if (data)
+    memcpy(ctx->data, data[0] + offset * m_format.m_frameSize, dataLength);
+  else
+    CAEUtil::GenerateSilence(m_format.m_dataFormat, m_format.m_frameSize, ctx->data, frames);
+
+  XAUDIO2_BUFFER xbuffer{};
+  xbuffer.AudioBytes = dataLength;
+  xbuffer.pAudioData = ctx->data;
+  xbuffer.pContext = ctx;
+
+  return xbuffer;
+}

xbmc/cores/AudioEngine/Sinks/AESinkXAudio.h

@@ -66,14 +66,27 @@ class CAESinkXAudio : public IAESink
         mBufferEnd.reset(CreateEventEx(nullptr, nullptr, 0, EVENT_MODIFY_STATE | SYNCHRONIZE));
         if (!mBufferEnd)
         {
-          throw std::exception("CreateEvent");
+          throw std::exception("CreateEventEx BufferEnd");
         }
+        if (NULL == (m_StreamEndEvent = CreateEventEx(nullptr, nullptr, CREATE_EVENT_MANUAL_RESET,
+                                                      EVENT_MODIFY_STATE | SYNCHRONIZE)))
+        {
+          throw std::exception("CreateEventEx StreamEnd");
+        }
+      }
+      virtual ~VoiceCallback()
+      {
+        if (m_StreamEndEvent != NULL)
+          CloseHandle(m_StreamEndEvent);
       }
-      virtual ~VoiceCallback() { }
 
       STDMETHOD_(void, OnVoiceProcessingPassStart) (UINT32) override {}
       STDMETHOD_(void, OnVoiceProcessingPassEnd)() override {}
-      STDMETHOD_(void, OnStreamEnd)() override {}
+      STDMETHOD_(void, OnStreamEnd)() override
+      {
+        if (m_StreamEndEvent != NULL)
+          SetEvent(m_StreamEndEvent);
+      }
       STDMETHOD_(void, OnBufferStart)(void*) override {}
       STDMETHOD_(void, OnBufferEnd)(void* context) override
       {
@@ -95,10 +108,25 @@ class CAESinkXAudio : public IAESink
         }
       };
       std::unique_ptr<void, handle_closer> mBufferEnd;
+      HANDLE m_StreamEndEvent{0};
     };
 
     bool InitializeInternal(std::string deviceId, AEAudioFormat &format);
     bool IsUSBDevice();
+    /*!
+     * \brief Add a 1 frame long buffer with the end of stream flag to the voice.
+     * \return true for success, false for failure
+     */
+    bool AddEndOfStreamPacket();
+    /*!
+     * \brief Create a XAUDIO2_BUFFER with a struct buffer_ctx in pContext member, which must
+     * be deleted either manually or by XAudio2 BufferEnd callbak to avoid memory leaks.
+     * \param data data of the frames to copy. if null, the new buffer will contain silence.
+     * \param frames number of frames
+     * \param offset offset from the start in the data buffer.
+     * \return the new buffer
+     */
+    XAUDIO2_BUFFER BuildXAudio2Buffer(uint8_t** data, unsigned int frames, unsigned int offset);
 
     Microsoft::WRL::ComPtr<IXAudio2> m_xAudio2;
     IXAudio2MasteringVoice* m_masterVoice;

xbmc/cores/AudioEngine/Utils/AEUtil.cpp

@@ -607,3 +607,25 @@ int CAEUtil::GetAVChannelIndex(enum AEChannel aechannel, uint64_t layout)
   av_channel_layout_uninit(&ch_layout);
   return idx;
 }
+
+void CAEUtil::GenerateSilence(AEDataFormat format,
+                              unsigned int frameSize,
+                              void* buffer,
+                              unsigned int frames)
+
+{
+  const unsigned int dataLength{frames * frameSize};
+
+  switch (format)
+  {
+    case AV_SAMPLE_FMT_U8:
+    case AV_SAMPLE_FMT_U8P:
+      memset(buffer, 0x80, dataLength);
+      break;
+
+    default:
+      // binary representation of 0 in all other formats regardless of number of bits per sample
+      // is a concatenation of 0 bytes.
+      memset(buffer, 0, dataLength);
+  }
+}

xbmc/cores/AudioEngine/Utils/AEUtil.h

@@ -178,4 +178,8 @@ class CAEUtil
   static uint64_t GetAVChannelMask(enum AEChannel aechannel);
   static enum AVChannel GetAVChannel(enum AEChannel aechannel);
   static int GetAVChannelIndex(enum AEChannel aechannel, uint64_t layout);
+  static void GenerateSilence(AEDataFormat format,
+                              unsigned int frameSize,
+                              void* buffer,
+                              unsigned int frames);
 };