Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

settings: Add settings option to control atempo resampling #10234

Merged
merged 1 commit into from Aug 14, 2016

Conversation

@popcornmix
Copy link
Member

commented Aug 5, 2016

Atempo rsampling does increase cpu usage so may want to be disabled (e.g. on Pi 1).
The threshold when atempo resampling is used compared to normal resampling could be usefully changed.

This adds a setting for controlling this and defaults to disabled on Pi 1

@@ -584,6 +585,7 @@ CActiveAEStreamBuffers::CActiveAEStreamBuffers(AEAudioFormat inputFormat, AEAudi
m_inputFormat = inputFormat;
m_resampleBuffers = new CActiveAEBufferPoolResample(inputFormat, outputFormat, quality);
m_atempoBuffers = new CActiveAEBufferPoolAtempo(outputFormat);
m_atempoThreshold = CSettings::GetInstance().GetInt(CSettings::SETTING_AUDIOOUTPUT_ATEMPOTHRESHOLD) / 100.0;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 5, 2016

Member

audio settings are managed by AE, loaded by CActiveAE::LoadSettings()
m_atempoThreshold can be set by CActiveAE::CreateStream

@@ -2370,6 +2370,16 @@
</constraints>
<control type="list" format="string" />
</setting>
<setting id="audiooutput.atempothreshold" type="integer" label="13517" help="13518">
<level>2</level>

This comment has been minimized.

Copy link
@MartijnKaijser

MartijnKaijser Aug 5, 2016

Member

Shouldn't it be an advanced setting?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 5, 2016

Member

Could be even an expert setting

@popcornmix popcornmix force-pushed the popcornmix:atemposetting branch from 031fcc0 to 4376e9d Aug 9, 2016
@popcornmix

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2016

@FernetMenta updated. Now expert setting and setting is set in CActiveAE::LoadSettings.

#empty strings from id 13517 to 13549
#: system/settings/settings.xml
msgctxt "#13517"
msgid "Threshold when atempo resampling kicks in"

This comment has been minimized.

Copy link
@garbear

garbear Aug 9, 2016

Member

Can we remove the word atempo from the GUI? This will confuse users, and googling returns no results

This comment has been minimized.

Copy link
@garbear

garbear Aug 9, 2016

Member

Suggestions welcome.
"Threshold when pitch preserving resampling is applied"?

How about "Speed threshold to rescale pitch"

And help text "Above this speed, audio is resampled to preserve the pitch. This has a minor performance impact."

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 9, 2016

Member

This is wrong in almost every sense :)

Above this speed

threshold is no speed

audio is resampled to preserve the pitch

it is the other way round. resampling does not preserve the pitch

This has a minor performance impact

we don't have any measurements on how atempo filter compares to resampling. apart from pi both is done by CPU.

This comment has been minimized.

Copy link
@garbear

garbear Aug 9, 2016

Member

Then let my ignorance be an example of one of those confused users I mentioned :) The takeaway is that this needs to be explained much, much better.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 9, 2016

Member

This is not a proposed help text but some background info:

For minor speed adjustments resampling is the preferred method. In regard to a/v sync it is more accurate. I am not completely sure but I think audio quality is also better.
For larger speed adjustments resampling is not ideal because it changes the pitch -> chipmunk voices. With v17 we introduced ffmpeg's atempo filter but we don't have too much experience with it yet. The atempo filter tries to keep the audio pitch.

This comment has been minimized.

Copy link
@garbear

garbear Aug 9, 2016

Member

Thanks for the explanation. Instead of a "pitch preserving resample algorithm", i would call this feature "pitch correction". So a better title text would be "Threshold for pitch correction".

The help text is still a little confusing to me. Is "resample ratio" just the speed factor, e.g. a 2% resample ratio is a 2% increase/decrease in speed?

This comment has been minimized.

Copy link
@popcornmix

popcornmix Aug 9, 2016

Author Member

Yes, we are referring to a 2% increase/decrease in speed as the threshold.
Technically that is a resample ratio of 98%/102%

This comment has been minimized.

Copy link
@garbear

garbear Aug 9, 2016

Member

ok, title text: "Threshold for pitch correction"

help text: "When the speed change exceeds this threshold, a pitch-correction filter will be applied. This avoids the "chipmonk voices" that normally result from speeding up a video."

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 10, 2016

Member

Thanks. I am ok with this.

@popcornmix

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2016

Suggestions welcome.
"Threshold when pitch preserving resampling is applied"?

@@ -716,7 +718,7 @@ bool CActiveAEStreamBuffers::IsDrained()

void CActiveAEStreamBuffers::SetRR(double rr)
{
if (rr < 1.02 && rr > 0.98)
if (fabs(rr - 1.0) < m_atempoThreshold)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 9, 2016

Member

if we passed threshold as parameter of SetRR instead of constructor, users would be able to change threshold on the fly. just add the setting to CActiveAE::OnSettingsChange

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 9, 2016

@popcornmix thanks, almost done. see my comment

@popcornmix popcornmix force-pushed the popcornmix:atemposetting branch from 4376e9d to b7e29bd Aug 9, 2016
@popcornmix

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2016

@FernetMenta updated

I did a quick profile. ActiveAE thread on Pi3 takes:
gpu resampling: 5%
low resampling: 15%
medium resampling 20%
high resampling 35%
atempo resampling: 15%

So it appears the atempo resampling is actually cheaper than medium resampling, but stopping the use of gpu resampling is the problem for Pi1.

I wonder if the audio quality of atempo resampling is noticeably lower for the audiophiles?

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 10, 2016

I wonder if the audio quality of atempo resampling is noticeably lower for the audiophiles?

Ask them and they answer yes. Just change an they won't notice :)

Thanks for your good work here!

Atempo rsampling does increase cpu usage so may want to be disabled (e.g. on Pi 1).
The threshold when atempo resampling is used compared to normal resampling could be usefully changed.

This adds a setting for controlling this and defaults to disabled on Pi 1
@popcornmix popcornmix force-pushed the popcornmix:atemposetting branch from b7e29bd to a05d418 Aug 10, 2016
@popcornmix

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2016

Updated with @garbear's suggested wording (which looks good to me).

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 12, 2016

jenkins build this please

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 14, 2016

Merge?

@FernetMenta FernetMenta merged commit c286f30 into xbmc:master Aug 14, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins.kodi.tv You did a great job. Have a cookie.
Details
@popcornmix popcornmix deleted the popcornmix:atemposetting branch Aug 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.