Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

change AE definition and usage of GetCacheTime to match how it is used in players #1131

Merged
merged 1 commit into from

7 participants

@davilla
Collaborator

Major bork in AE, the def/usage of GetCacheTime is wrong with respect to the players that call AE. Players rule so AE needs to change.

Engines:
SoftAE usage is correct with this pull/req.
PulseAE needs checking.
CoreAudio usage is correct.

SoftAE Sinks:
AESinkALSA looks wrong, someone familiar with ALSA needs to help fix it.
AESinkOSS looks wrong, someone familiar with OSS needs to help fix it.
AESinkNULL is totally borked and needs fixing.
AESinkWASAPI needs checking and fixing if required.
AESikkDirectSound usage is correct.

@jmarshallnz
Owner

Code looks good.

@DDDamian, @chadoe, @WiSo, Need your signoff on WASAPI - DirectSound looks OK, but WASAPI looks like GetDelay and GetCacheTime need swapping around? GetCacheTime should return the amount in seconds in the buffer, GetDelay should include this plus any latency in the hardware.

@gnif, @FernetMenta your signoff for ALSA would be good.

@cptspiff, @topfs2 your signoff on Pulse would be good.

@FernetMenta
Collaborator

I am not 100% clear about the comments in the header. GetDelay(): Is it the time it takes for the previously added frame to get audible or the next frame? There is no measure to determine how long it takes to get the next frame into the buffer of the audio device (buffer might be full). I would expect the calculation takes the previous frame into account.
Is this correct?
GetDelay() - GetCacheTime() = hw latency

@elupus
Collaborator

GetDelay() - GetCacheTime() = hw latency + buffers in receivers or tvs and so on.

GetDelay() is current situation. Ie what will apply if you add a frame directly after polling that function.

It would technically be better if we had returned a struct with absolute timestamps instead. But that is for another day.

Pull looks good to me.

@elupus
Collaborator

Pulse looks correct already.

@davilla
Collaborator

I'd like to get this in for this pull cycle, no doubt it might cause all sorts of unseen effects that will need to get tracked down and fixed. ALSA and WASAPI are the remaining holdouts.

@elupus
Collaborator

Alsa looks correct

Wasapi looks wrong for all the functions:

GetDelay uses GetBufferSize: "The GetBufferSize method retrieves the size (maximum capacity) of the endpoint buffer."

GetCacheTotal and GetCacheTime uses GetStreamLatency: "This method retrieves the maximum latency for the current stream. The value will not change for the lifetime of the IAudioClient object." "Rendering clients can use this latency value to compute the minimum amount of data that they can write during any single processing pass. To write less than this minimum is to risk introducing glitches into the audio stream."

@elupus
Collaborator

GetStreamLatency would have been used to setup GetChunkSize() in the old system. Ie the minimum amount of data to write each time. But seems that logic isn't there for AE.

@elupus
Collaborator

"For a rendering buffer, the padding value that is reported by the IAudioClient::GetCurrentPadding method represents the amount of rendering data that is queued up to play in the buffer" so there we have an okey implementation for GetCacheTime() and GetDelay(). It's possible latency should be added to that for the GetDelay() call. But not sure.

@chadoe
Collaborator

so for wasapi that would become:

double CAESinkWASAPI::GetDelay()
{
    return GetCacheTime();
}

double CAESinkWASAPI::GetCacheTime()
{
  HRESULT hr;
  if (!m_initialized)
    return 0.0;

  unsigned int numPaddingFrames;
  hr = m_pAudioClient->GetCurrentPadding(&numPaddingFrames);
  if (FAILED(hr))
  {
    #ifdef _DEBUG
    CLog::Log(LOGERROR, __FUNCTION__": GetCurrentPadding Failed : %s", WASAPIErrToStr(hr));
    #endif
    return 0.0;
  }
  return (double)numPaddingFrames / (double)m_format.m_sampleRate;
}
@elupus
Collaborator
@davilla
Collaborator

will do

@FernetMenta
Collaborator

I might be wrong but I think there is an additional problem with regard to e.g. CSoftAE::GetDelay
I don't see the buffers being protected by locks. In particular when transcoding is active I do observe a lot of discontinuity errors caused by audio. What happens when data is taken out of input buffers and GetDealy is called before this data is pushed into another buffer?

@davilla
Collaborator

Locks around GetDelay would be the responsibility of the specific sink implementation if required. I'd be very careful there to avoid a deadlock as there could two threads accessing GetDelay, Typically paplayer or dvdaudio are the ones that access it, but when the OSD is up, then it also gets this info though an indirect call via dvdaudio.

Note that if you are running under linux, CThread priorities are messed up and that's the subject of another pull/req that is brewing.

@davilla
Collaborator

CAESinkWASAPI changed. If everyone is happy now, I'll combine this with the original, force push and merge into mainline.

@davilla davilla merged commit 8ac5a86 into from
@DDDamian DDDamian commented on the diff
xbmc/cores/AudioEngine/Sinks/AESinkWASAPI.cpp
@@ -352,11 +340,14 @@ double CAESinkWASAPI::GetCacheTime()
if (!m_initialized)
return 0.0;
- REFERENCE_TIME hnsLatency;
- HRESULT hr = m_pAudioClient->GetStreamLatency(&hnsLatency);
-
- /** returns buffer duration in seconds */
- return hnsLatency / 10.0;
+ unsigned int numPaddingFrames;
+ HRESULT hr = m_pAudioClient->GetCurrentPadding(&numPaddingFrames);
@DDDamian Collaborator

GetCurrentPadding in exclusive mode event-driven always returns the size of one complete buffer (there are two) per M$. I did write a very extensive routine that polled the hardware pointer within the current buffer and used that plus the current performance clock to track this very accurately, but never upped it for a few reasons:
1) the required return value for of GetCacheTime() wasn't clear
2) being polled every ~2ms it was just added overhead considering:
3) it seemed to have zero effect on the perceived or reported OSD sync error

Regardless, good that there's some clarity/consistency now as to the purpose of this function - good work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnova

Since this seems to change the behavior of GetCacheTime() shouldn't DVDPlayer & PAPlayer be updated accordingly?

Collaborator

DVDPlayer & PAPlayer use this def of GetCacheTime already, that was the whole point of changing AE, it did not match the usage in players which has never changed.

Collaborator
@LongChair LongChair referenced this pull request from a commit in plexinc/plex-home-theater-public
@LongChair LongChair Specify the proper type when retrieving genres. fixes #1131 205b669
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 10, 2012
  1. @davilla
This page is out of date. Refresh to see the latest.
View
2  xbmc/cores/AudioEngine/Engines/SoftAE/SoftAE.cpp
@@ -809,7 +809,7 @@ double CSoftAE::GetCacheTime()
CSharedLock sinkLock(m_sinkLock);
double time;
- time = (double)m_buffer.Free() * m_sinkFormatFrameSizeMul * m_sinkFormatSampleRateMul;
+ time = (double)m_buffer.Used() * m_sinkFormatFrameSizeMul * m_sinkFormatSampleRateMul;
time += m_sink->GetCacheTime();
return time;
View
4 xbmc/cores/AudioEngine/Engines/SoftAE/SoftAEStream.cpp
@@ -489,8 +489,8 @@ double CSoftAEStream::GetCacheTime()
return 0.0;
double time;
- time = (double)(m_inputBuffer.Free() / m_format.m_frameSize) / (double)m_format.m_sampleRate;
- time += (double)(m_waterLevel - m_refillBuffer) / (double)AE.GetSampleRate();
+ time = (double)(m_inputBuffer.Used() / m_format.m_frameSize) / (double)m_format.m_sampleRate;
+ time += (double)(m_waterLevel - m_framesBuffered) / (double)AE.GetSampleRate();
time += AE.GetCacheTime();
return time;
}
View
10 xbmc/cores/AudioEngine/Interfaces/AESink.h
@@ -53,22 +53,24 @@ class IAESink
virtual bool IsCompatible(const AEAudioFormat format, const std::string device) = 0;
/*
- This method must return the delay in seconds till new data will be sent out
+ This method returns the time in seconds that it will take
+ for the next added packet to be heard from the speakers.
*/
virtual double GetDelay() = 0;
/*
- This method returns the time in seconds till the sink's cache is full
+ This method returns the time in seconds that it will take
+ to underrun the cache if no sample is added.
*/
virtual double GetCacheTime() = 0;
/*
- This method returns the total length of the cache in seconds
+ This method returns the total time in seconds of the cache.
*/
virtual double GetCacheTotal() = 0;
/*
- Adds packets to be sent out, must block after at-least one block is being rendered
+ Adds packets to be sent out, this routine MUST block or sleep.
*/
virtual unsigned int AddPackets(uint8_t *data, unsigned int frames) = 0;
View
14 xbmc/cores/AudioEngine/Interfaces/AEStream.h
@@ -59,8 +59,9 @@ class IAEStream
virtual unsigned int AddData(void *data, unsigned int size) = 0;
/**
- * Returns how long until new data will be played
- * @return The delay in seconds
+ * Returns the time in seconds that it will take
+ * for the next added packet to be heard from the speakers.
+ * @return seconds
*/
virtual double GetDelay() = 0;
@@ -71,14 +72,15 @@ class IAEStream
virtual bool IsBuffering() = 0;
/**
- * Returns how long until playback will start
- * @return The delay in seconds
+ * Returns the time in seconds that it will take
+ * to underrun the cache if no sample is added.
+ * @return seconds
*/
virtual double GetCacheTime() = 0;
/**
- * Returns the total length of the cache before playback will start
- * @return The delay in seconds
+ * Returns the total time in seconds of the cache
+ * @return seconds
*/
virtual double GetCacheTotal() = 0;
View
27 xbmc/cores/AudioEngine/Sinks/AESinkWASAPI.cpp
@@ -332,19 +332,7 @@ bool CAESinkWASAPI::IsCompatible(const AEAudioFormat format, const std::string d
double CAESinkWASAPI::GetDelay()
{
- HRESULT hr;
- if (!m_initialized)
- return 0.0;
-
- hr = m_pAudioClient->GetBufferSize(&m_uiBufferLen);
- if (FAILED(hr))
- {
- #ifdef _DEBUG
- CLog::Log(LOGERROR, __FUNCTION__": GetBufferSize Failed : %s", WASAPIErrToStr(hr));
- #endif
- return 0.0;
- }
- return (double)m_uiBufferLen / (double)m_format.m_sampleRate;
+ return GetCacheTime();
}
double CAESinkWASAPI::GetCacheTime()
@@ -352,11 +340,14 @@ double CAESinkWASAPI::GetCacheTime()
if (!m_initialized)
return 0.0;
- REFERENCE_TIME hnsLatency;
- HRESULT hr = m_pAudioClient->GetStreamLatency(&hnsLatency);
-
- /** returns buffer duration in seconds */
- return hnsLatency / 10.0;
+ unsigned int numPaddingFrames;
+ HRESULT hr = m_pAudioClient->GetCurrentPadding(&numPaddingFrames);
@DDDamian Collaborator

GetCurrentPadding in exclusive mode event-driven always returns the size of one complete buffer (there are two) per M$. I did write a very extensive routine that polled the hardware pointer within the current buffer and used that plus the current performance clock to track this very accurately, but never upped it for a few reasons:
1) the required return value for of GetCacheTime() wasn't clear
2) being polled every ~2ms it was just added overhead considering:
3) it seemed to have zero effect on the perceived or reported OSD sync error

Regardless, good that there's some clarity/consistency now as to the purpose of this function - good work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ if (FAILED(hr))
+ {
+ CLog::Log(LOGERROR, __FUNCTION__": GetCurrentPadding Failed : %s", WASAPIErrToStr(hr));
+ return 0.0;
+ }
+ return (double)numPaddingFrames / (double)m_format.m_sampleRate;
}
double CAESinkWASAPI::GetCacheTotal()
Something went wrong with that request. Please try again.