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

Merged
merged 1 commit into from Jul 10, 2012

Conversation

Projects
None yet
7 participants
Contributor

davilla commented Jul 7, 2012

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.

Member

jmarshallnz commented Jul 7, 2012

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.

Member

FernetMenta commented Jul 8, 2012

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

Member

elupus commented Jul 8, 2012

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.

Member

elupus commented Jul 8, 2012

Pulse looks correct already.

Contributor

davilla commented Jul 8, 2012

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.

Member

elupus commented Jul 8, 2012

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."

Member

elupus commented Jul 8, 2012

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.

Member

elupus commented Jul 8, 2012

"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.

Member

chadoe commented Jul 8, 2012

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;
}
Member

elupus commented Jul 8, 2012

Should be fine. But drop the ifdefs around the log. We want to know errors.

Contributor

davilla commented Jul 8, 2012

will do

Member

FernetMenta commented Jul 9, 2012

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?

Contributor

davilla commented Jul 9, 2012

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.

Contributor

davilla commented Jul 10, 2012

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

@davilla davilla added a commit that referenced this pull request Jul 10, 2012

@davilla davilla Merge pull request #1131 from davilla/fix-ae-getcachetime
change AE definition and usage of GetCacheTime to match how it is used in players
8ac5a86

@davilla davilla merged commit 8ac5a86 into xbmc:master Jul 10, 2012

@DDDamian DDDamian commented on the diff Jul 11, 2012

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

DDDamian Jul 11, 2012

Contributor

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.

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

Contributor

davilla replied Jul 14, 2012

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.

Member

elupus replied Jul 14, 2012

@LongChair LongChair added a commit to plexinc/plex-home-theater-public that referenced this pull request Dec 15, 2014

@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