AE: Revisit Period Size, Period and Buffersize #2320

Merged
merged 1 commit into from Mar 5, 2013

Conversation

Projects
None yet
9 participants
@fritsch
Member

fritsch commented Feb 28, 2013

First Part:
The triangle of this linear correlation was fixed to a constant Period of 16 and the Buffersize was limited to 8192. Therefore Period Size could be as low as 512. This is too small some specific hardware and results in stutter. We now use the 1024 as Period Size and use what we get as Buffersize after setting Period Size. We let the hw decide on the needed Period time.

Second part:
The initialization is now done with _near methods, so this should make sure, that we get the Sink initialized with one try.

Testing on other hardware would be a good idea.

@davilla

This comment has been minimized.

Show comment Hide comment
@davilla

davilla Feb 28, 2013

Contributor

+1, works great on both AMLogic M1/M3 based SoCs under Pivos/Linux.

Contributor

davilla commented Feb 28, 2013

+1, works great on both AMLogic M1/M3 based SoCs under Pivos/Linux.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Mar 1, 2013

Member

That can cause problems for dvd player. You are requesting a buffer of
256/48 seconds. Ie 5 seconds. That will cause a huge skew between audio and
video in dvd player. Imho trying to set it to 1 second would be better.
Also you aught to take sample rate into account.

Try just setting it to sample rate.

Period could still be large. For example samplerate divided by 10 (100ms)

Member

elupus commented Mar 1, 2013

That can cause problems for dvd player. You are requesting a buffer of
256/48 seconds. Ie 5 seconds. That will cause a huge skew between audio and
video in dvd player. Imho trying to set it to 1 second would be better.
Also you aught to take sample rate into account.

Try just setting it to sample rate.

Period could still be large. For example samplerate divided by 10 (100ms)

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 1, 2013

Member

@elupus:
I think I cannot follow here directly. Basically we choose what the Sink wants to give us and have an upper maximum of 256K in order to not get too far away. Basically, we can change the line as follows:

bufferSize  = std::min(bufferSize, (snd_pcm_uframes_t) 64 * 1024);

Did I miss the point?

Member

fritsch commented Mar 1, 2013

@elupus:
I think I cannot follow here directly. Basically we choose what the Sink wants to give us and have an upper maximum of 256K in order to not get too far away. Basically, we can change the line as follows:

bufferSize  = std::min(bufferSize, (snd_pcm_uframes_t) 64 * 1024);

Did I miss the point?

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 1, 2013

Member

I missed the point. You are right, it is a good idea to buffer 1 second, which corresponds to the sample rate value. Will add a patch tonight.

bufferSize  = std::min(bufferSize, (snd_pcm_uframes_t) sampleRate * channelCount );`
Member

fritsch commented Mar 1, 2013

I missed the point. You are right, it is a good idea to buffer 1 second, which corresponds to the sample rate value. Will add a patch tonight.

bufferSize  = std::min(bufferSize, (snd_pcm_uframes_t) sampleRate * channelCount );`
@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 1, 2013

Member

Now I think, if we don't need to calc at all, cause our sink is ready set up at this position, so we just could use:

snd_pcm_hw_params_set_buffer_time((m_pcm, hw_params, 1000 * 1000, NULL);

Will test it at home

Member

fritsch commented Mar 1, 2013

Now I think, if we don't need to calc at all, cause our sink is ready set up at this position, so we just could use:

snd_pcm_hw_params_set_buffer_time((m_pcm, hw_params, 1000 * 1000, NULL);

Will test it at home

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 1, 2013

Member

@elupus:
I recalced the values and came to similar results as you did above. Though setting it to buffer a whole second cannot be done by all hw I think. I set it to double the period Size.

Member

fritsch commented Mar 1, 2013

@elupus:
I recalced the values and came to similar results as you did above. Though setting it to buffer a whole second cannot be done by all hw I think. I set it to double the period Size.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Mar 1, 2013

Member

double period size is too small I think. We want to have some time in dvdplayer to fill up new data. one second or half a second should be good enough. But don't make it much smaller.

Member

elupus commented Mar 1, 2013

double period size is too small I think. We want to have some time in dvdplayer to fill up new data. one second or half a second should be good enough. But don't make it much smaller.

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 1, 2013

Member

Oki 2 is 200ms, 4 is 400 ms, 5 looks ugly, so let's try 8?

Btw. if you calc that values by hand on think of a 192khz source. that is a whole lot of data we are getting here...

Member

fritsch commented Mar 1, 2013

Oki 2 is 200ms, 4 is 400 ms, 5 looks ugly, so let's try 8?

Btw. if you calc that values by hand on think of a 192khz source. that is a whole lot of data we are getting here...

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Mar 1, 2013

Member

fine be me.

Member

elupus commented Mar 1, 2013

fine be me.

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 1, 2013

Member

Squashed and rebased.

Member

fritsch commented Mar 1, 2013

Squashed and rebased.

@gnif

This comment has been minimized.

Show comment Hide comment
@gnif

gnif Mar 2, 2013

Member

The messy buffer size logic is REQUIRED due to issues with ALSA driver implementations, you are re-introducing a bug where the buffer size never gets set on some hardware. For confirmation please check the pulse audio ALSA driver where they have had to do the same thing.

Member

gnif commented Mar 2, 2013

The messy buffer size logic is REQUIRED due to issues with ALSA driver implementations, you are re-introducing a bug where the buffer size never gets set on some hardware. For confirmation please check the pulse audio ALSA driver where they have had to do the same thing.

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 2, 2013

Member

@gnif: Thx for jumping in. So back to the roots or can we change something here, that increases period size to at least 1024 (fixes @davilla issue) e.g. the original patch, that started with period_size = 1024 and let the hw decide to use a buffer? Also see, that all hw access functions use snd_pcm_hw_params_set_buffer_size_near so I expected that at least something gets set.

Member

fritsch commented Mar 2, 2013

@gnif: Thx for jumping in. So back to the roots or can we change something here, that increases period size to at least 1024 (fixes @davilla issue) e.g. the original patch, that started with period_size = 1024 and let the hw decide to use a buffer? Also see, that all hw access functions use snd_pcm_hw_params_set_buffer_size_near so I expected that at least something gets set.

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 2, 2013

Member

I think I found a way, that does what the old logic does, but a bit more transparent, as we know which one failed and have to try only once for buffer_size.

Member

fritsch commented Mar 2, 2013

I think I found a way, that does what the old logic does, but a bit more transparent, as we know which one failed and have to try only once for buffer_size.

@gnif

This comment has been minimized.

Show comment Hide comment
@gnif

gnif Mar 3, 2013

Member

@fritsch: No worries, there is a bit of black magic and voodoo with a pinch of salt in here, I would get this tested by the community before assuming it is correct. You also need to work with a copy of the hwparams as each attempt modifies values in the structure in ALSA which can cause the next attempt to fail.

Member

gnif commented Mar 3, 2013

@fritsch: No worries, there is a bit of black magic and voodoo with a pinch of salt in here, I would get this tested by the community before assuming it is correct. You also need to work with a copy of the hwparams as each attempt modifies values in the structure in ALSA which can cause the next attempt to fail.

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 3, 2013

Member

The last commit was done with the kind help of @anssih - highly appreciated. Thx.

Member

fritsch commented Mar 3, 2013

The last commit was done with the kind help of @anssih - highly appreciated. Thx.

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 4, 2013

Member

@elupus @davilla @gnif
Okay for you all so far?

Member

fritsch commented Mar 4, 2013

@elupus @davilla @gnif
Okay for you all so far?

@davilla

This comment has been minimized.

Show comment Hide comment
@davilla

davilla Mar 5, 2013

Contributor

seems ok on pivos m/1/m3, inject ?

Contributor

davilla commented Mar 5, 2013

seems ok on pivos m/1/m3, inject ?

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 5, 2013

Member

Let's wait on @elupus as it is also important for his dvdplayer, so we don't have to change stuff afterwards, is this okay for you?

Member

fritsch commented Mar 5, 2013

Let's wait on @elupus as it is also important for his dvdplayer, so we don't have to change stuff afterwards, is this okay for you?

@gnif

This comment has been minimized.

Show comment Hide comment
@gnif

gnif Mar 5, 2013

Member

@fritsch - Code looks good, but unable to test for the moment

Member

gnif commented Mar 5, 2013

@fritsch - Code looks good, but unable to test for the moment

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Mar 5, 2013

Member

Fine by me as long as it guarantee half a sec buffer or there about. The
details on the workaround I'm not especially qualified to answer.

Member

elupus commented Mar 5, 2013

Fine by me as long as it guarantee half a sec buffer or there about. The
details on the workaround I'm not especially qualified to answer.

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 5, 2013

Member

@elupus: We want to get 8 times period size, which is (the period size) always 100ms. If we see we can only get 8192 frames buffer, we reduce the period size to half the buffer in order not to run into underruns, to have enough data whenever a period ends.

Member

fritsch commented Mar 5, 2013

@elupus: We want to get 8 times period size, which is (the period size) always 100ms. If we see we can only get 8192 frames buffer, we reduce the period size to half the buffer in order not to run into underruns, to have enough data whenever a period ends.

davilla added a commit that referenced this pull request Mar 5, 2013

Merge pull request #2320 from fritsch/ae-period-size
AE: Revisit Period Size, Period and Buffersize

@davilla davilla merged commit 3644e98 into xbmc:master Mar 5, 2013

@ronie

This comment has been minimized.

Show comment Hide comment
@ronie

ronie Mar 9, 2013

Member

meh...this is causing major audio borkage on my end.

paplayer + pulseaudio = underrun x many
http://xbmclogs.com/show.php?id=3069

all other combinations (dvdplayer + pulseaudio / paplayer + alsa) are fine.

Member

ronie commented Mar 9, 2013

meh...this is causing major audio borkage on my end.

paplayer + pulseaudio = underrun x many
http://xbmclogs.com/show.php?id=3069

all other combinations (dvdplayer + pulseaudio / paplayer + alsa) are fine.

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 9, 2013

Member

@ronie:
Please try that one on top: #2382 You have USB soundcard, right? Same issue I have seen with some OE testers.

Member

fritsch commented Mar 9, 2013

@ronie:
Please try that one on top: #2382 You have USB soundcard, right? Same issue I have seen with some OE testers.

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 9, 2013

Member

Yeah - no usb soundcard, but everything done in userspace via the pulseaudio default device.

Member

fritsch commented Mar 9, 2013

Yeah - no usb soundcard, but everything done in userspace via the pulseaudio default device.

@vicbitter

This comment has been minimized.

Show comment Hide comment
@vicbitter

vicbitter Mar 10, 2013

This PR breaks playback of DTS-HD and Dolby TrueHD on nVidia platforms...

This PR breaks playback of DTS-HD and Dolby TrueHD on nVidia platforms...

@sjongele

This comment has been minimized.

Show comment Hide comment
@sjongele

sjongele Mar 10, 2013

Fritsch, 2320 causes big underruns with DTS-HD MA and TrueHD passthrough on my system: Intel, Ubuntu 12.04 64-bit, NVidia GT540M, ALSA, no Pulse.

Fritsch, 2320 causes big underruns with DTS-HD MA and TrueHD passthrough on my system: Intel, Ubuntu 12.04 64-bit, NVidia GT540M, ALSA, no Pulse.

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 10, 2013

Member

@sjongele: Please retry with 2382 (just got merged) - this should fix that stuff. Buffer was way too large.

Member

fritsch commented Mar 10, 2013

@sjongele: Please retry with 2382 (just got merged) - this should fix that stuff. Buffer was way too large.

@vicbitter

This comment has been minimized.

Show comment Hide comment
@vicbitter

vicbitter Mar 10, 2013

@fritsch, on OpenELEC builds with PR 2382 still causes problems when playing DTS-HD and Dolby TrueHD.

@fritsch, on OpenELEC builds with PR 2382 still causes problems when playing DTS-HD and Dolby TrueHD.

@sjongele

This comment has been minimized.

Show comment Hide comment
@sjongele

sjongele Mar 10, 2013

Hello Fritsch; sorry, still lots of underruns. 2382 does not solve the issue.

Hello Fritsch; sorry, still lots of underruns. 2382 does not solve the issue.

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 10, 2013

Member

Oki - so I need logfiles. Best is before and after those patches.

Member

fritsch commented Mar 10, 2013

Oki - so I need logfiles. Best is before and after those patches.

@fritsch fritsch deleted the fritsch:ae-period-size branch Mar 10, 2013

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 10, 2013

Member

@sjongele:
fritsch/xbmc@4c8fe02

Please try this one on top of everything - let's discuss it further on my github, to not bomb too much people. I have underestimated the calculation of "sane" values. It rather looks, that we have to go back to what we had before.

Member

fritsch commented Mar 10, 2013

@sjongele:
fritsch/xbmc@4c8fe02

Please try this one on top of everything - let's discuss it further on my github, to not bomb too much people. I have underestimated the calculation of "sane" values. It rather looks, that we have to go back to what we had before.

@anssih

This comment has been minimized.

Show comment Hide comment
@anssih

anssih Mar 10, 2013

Member

I'd rather not go back just yet.

Please try if either of these fixes the issue:
https://github.com/anssih/xbmc/tree/fix/AE-buffersize
https://github.com/anssih/xbmc/tree/fix/AE-periodsize

If neither helps, before/after logs would be nice.

Member

anssih commented Mar 10, 2013

I'd rather not go back just yet.

Please try if either of these fixes the issue:
https://github.com/anssih/xbmc/tree/fix/AE-buffersize
https://github.com/anssih/xbmc/tree/fix/AE-periodsize

If neither helps, before/after logs would be nice.

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 11, 2013

Member

@sjongele:
Yes, please check the patches made by @anssih - it is too early to give up yet - and as xbmc currently is in dev phase - it is okay to break and work on better dynamic defaults. When you start testing with anssih/xbmc@95ea48d you can just cherry-pick the other anssih/xbmc@39f2487 on top.

Thanks for your help.

Member

fritsch commented Mar 11, 2013

@sjongele:
Yes, please check the patches made by @anssih - it is too early to give up yet - and as xbmc currently is in dev phase - it is okay to break and work on better dynamic defaults. When you start testing with anssih/xbmc@95ea48d you can just cherry-pick the other anssih/xbmc@39f2487 on top.

Thanks for your help.

@vicbitter

This comment has been minimized.

Show comment Hide comment
@vicbitter

vicbitter Mar 11, 2013

@fritsch , after applying both of @anssih's patches, I can confirm that DTS-HD and Dolby TrueHD are working again :)

Thanks for the quick turn around on a fix...

@fritsch , after applying both of @anssih's patches, I can confirm that DTS-HD and Dolby TrueHD are working again :)

Thanks for the quick turn around on a fix...

@Giftie

This comment has been minimized.

Show comment Hide comment
@Giftie

Giftie Mar 13, 2013

Contributor

I needed to add both to fix the problem on my side.

Contributor

Giftie commented Mar 13, 2013

I needed to add both to fix the problem on my side.

@fritsch

This comment has been minimized.

Show comment Hide comment
@fritsch

fritsch Mar 13, 2013

Member

@Giftie: Thx for your answer. This is also my experience: #2421 @anssih has PRd both and we need both. The important part is here that the bufferSize must be 4 times the periodSize -> so perhaps the second patch is enough alone - but applying a potential bufferSize without the influence of a first periodSize makes it more fail proof.

Both of those patches are in OpenELEC rc5 and for now - it looks good.

Member

fritsch commented Mar 13, 2013

@Giftie: Thx for your answer. This is also my experience: #2421 @anssih has PRd both and we need both. The important part is here that the bufferSize must be 4 times the periodSize -> so perhaps the second patch is enough alone - but applying a potential bufferSize without the influence of a first periodSize makes it more fail proof.

Both of those patches are in OpenELEC rc5 and for now - it looks good.

@sjongele

This comment has been minimized.

Show comment Hide comment
@sjongele

sjongele Mar 13, 2013

@fritsch
Not sure where my response ended up, but thought I had posted success with this solution as well.

@fritsch
Not sure where my response ended up, but thought I had posted success with this solution as well.

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