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

Remove audio frame limit. #667

Merged
merged 1 commit into from Jul 28, 2014

Conversation

RachelBryk
Copy link
Member

It serves no purpose and causes bugs and confusion for users.

@jezze
Copy link
Contributor

jezze commented Jul 24, 2014

I have no comment about the actual change because I know nothing about it but I like when commit messages explains the changes so I know why removing the frame limit is a good thing.

@MayImilae
Copy link
Contributor

Audio frame limit's purpose was to workaround fullspeed pops and crackling in async audio. Now that Audio is synced to the framerate in all circumstances, there's no reason for it anymore, and it can cause confusion among users. Hence, this PR.

And I agree that this should be in the description.

@@ -631,7 +631,7 @@ void VideoThrottle()
bool ShouldSkipFrame(int skipped)
{
const u32 TargetFPS = (SConfig::GetInstance().m_Framelimit > 1)
? SConfig::GetInstance().m_Framelimit * 5
? (SConfig::GetInstance().m_Framelimit - 1) * 5

This comment was marked as off-topic.

This comment was marked as off-topic.

@JMC47
Copy link
Contributor

JMC47 commented Jul 24, 2014

This is going to wreck that harvest moon game where the audio is running at 33 fps on HLE and 31 fps on LLE.

@pauldacheez
Copy link
Contributor

There's also other audio issues that I've seen people claim audio framelimit fixes (possibly involving the wide variety of frequencies near 48kHz that PC soundcards actually process instead of exactly 48kHz; check out higan's config for this stuff). It'd be best to figure out the cause of these and the Harvest Moon issues and fix them properly before removing the audio framelimit.

...Then again, 90% of the people currently using it probably shouldn't be using it, so we might as well remove it now.

@lioncash
Copy link
Member

@JMC47 How would it wreck it? (Don't know if this was an issue in the past or not)

@JMC47
Copy link
Contributor

JMC47 commented Jul 24, 2014

The only way to get the audio to sound half decent is to set frame-limit to audio and let it run at 33 fps/66 vps in HLE, and 31 fps, 62vps in LLE.

@lioncash
Copy link
Member

Wouldn't that be indicative of an underlying emulation issue though? Audio is one of the things I know least about Dolphin, so if I'm wrong, just tell me.

@JMC47
Copy link
Contributor

JMC47 commented Jul 24, 2014

Oh yeah, it's definitely some kind of bug in Dolphin. I'm just saying perhaps it should be looked into before we go taking away the only work-around for the issue. I don't want a case where people are using older builds like when the sonic unleashed hack was removed with no replacement.

@JMC47
Copy link
Contributor

JMC47 commented Jul 24, 2014

You know what? Nevermind. framelimit to audio no longer fixes this anyway, it got broken worse somewhere along the lines and I didn't notice.

LGTM

@RachelBryk
Copy link
Member Author

"fixing" one game isn't a reason to keep it anyway.

@phire
Copy link
Member

phire commented Jul 24, 2014

From what I can tell, audio frame limit was broken (further) in the Rewrite DTK merge, it would probably need a major rework to start working again.

No reason not to remove it.

@lioncash
Copy link
Member

Looks good to me.

@neobrain
Copy link
Member

@RachelBryk Please fix the description of the PR to actually explain why this feature should be removed. Not everyone wants to read the whole thread just to figure out why this is even happening.

It serves no purpose and causes bugs and confusion for users.
@JMC47
Copy link
Contributor

JMC47 commented Jul 27, 2014

Audio Framelimit is really messed up in the very latest builds, sticking to the wrong framerates even in most games. So, I think it should be removed asap.

@neobrain
Copy link
Member

It would have been nice if the reasoning for this had been presented better, but since @JMC47 says that it's rather broken than useful I guess this change is for the better.

neobrain added a commit that referenced this pull request Jul 28, 2014
@neobrain neobrain merged commit 9c7d4b6 into dolphin-emu:master Jul 28, 2014
@Oberworld
Copy link

hi, I have problems since this commit, I was playing metroid prime 2: echoes and since this commit I can't play because of the very very clipping sound, it's very annoying. I adjust fps for PAL version (50fps), HLE, LLE but nothing helps.

With audio frame limit sound is good in this game with no clipping for me.

Anyway only have noticed in this game, others games seems to be ok.

@JMC47
Copy link
Contributor

JMC47 commented Jul 31, 2014

Could you make an issue report with audio dumps. If the game has problems with audio, then we'd gladly look at it. Make sure you do not have the vbeam speedhack on while testing.

http://code.google.com/p/dolphin-emu/issues/list

Audio frame-limit should never be needed for a game to have working audio.

@Oberworld
Copy link

Thanks for your quick reply.
Yes, after some testing, disabling Vbeam solved the problem.
Thankyou very much and keep the good work.
Regards.

@RachelBryk RachelBryk deleted the remove-audio-limit branch September 22, 2014 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
10 participants