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

AUDIO: Add support for sample rates >65kHz. #348

Closed
wants to merge 1 commit into from

Conversation

@fuzzie
Copy link
Member

fuzzie commented Jun 30, 2013

This is needed for 96kHz ogg files in the AGS game The Blackwell Legacy (1.7).

The ARM code isn't updated by this yet.

@sev-
Copy link
Member

sev- commented Jul 1, 2013

I have a feeling that you may be losing precision. Did you consider employing a running average instead of silently throwing the samples out?

@fuzzie
Copy link
Member Author

fuzzie commented Jul 1, 2013

Yes, this is losing precision, and although it's not audible with my test case, it's not a nice solution.. After some more consideration I think a better solution might be to just reduce the accuracy of the interpolation of the linear converter slightly (to 15-bit precision). I'll update this pull request with that alternative later.

@fuzzie
Copy link
Member Author

fuzzie commented Jul 2, 2013

This new commit (reducing the fractional bits used in the rate code to 15) works fine for my test case, and I think/hope it's fine, although we should sync up the ARM code too.

@sev-
Copy link
Member

sev- commented Jul 5, 2013

Looks fine now.

@lordhoto
Copy link
Member

lordhoto commented Jul 5, 2013

Looks fine to me too. But I would rename the pull request before merging it. The "hack" bit could be a confusion source later on.

@lordhoto
Copy link
Member

lordhoto commented Jul 9, 2013

@fuzzie Do you think we should fix the ARM code before merging this or afterwards?

@bluegr
Copy link
Member

bluegr commented Jul 15, 2013

IMHO, adapting the ARM code could be done afterwards. I mean, there's no reason to hold this working solution in queue while waiting for someone to adapt the other relevant ARM code

@lordhoto
Copy link
Member

lordhoto commented Jul 18, 2013

fuzzie wants to have the ARM code fixed first. I think that makes sense since it avoids any chances of the ARM code being forgotten. Furthermore, this change is only needed for code not in-tree. Thus, delaying the merge is really no issue at all.

@bluegr
Copy link
Member

bluegr commented Nov 22, 2013

Just a note: This is needed by the Wintermute game Shaban, which is trying to use a 96000Hz sample

@somaen
Copy link
Member

somaen commented Jan 27, 2014

Any news on this?

@digitall
Copy link
Member

digitall commented Jun 1, 2014

This is now needed for the Wintermute game "Helga Deep in Trouble". See:
http://sourceforge.net/p/scummvm/bugs/6580/

Will take a look at the required ARM asm changes.

@digitall
Copy link
Member

digitall commented Jun 1, 2014

Have made the required changes as far as I understand them... based on comparing fuzzie's changes and applying them across to the ARM code. Totally untested, so any review of the changes is good... If these can get incorporated and tested, then we can merge and close this:
https://github.com/digitall/scummvm/compare/rate-hack
digitall@9003ce5

@RichieSams RichieSams force-pushed the scummvm:master branch from 807d18e to 2ec3adf Sep 12, 2014
@digitall digitall force-pushed the scummvm:master branch from 2208eda to 5b5a2bc Oct 26, 2014
@bluegr
Copy link
Member

bluegr commented Oct 29, 2014

I've checked @digitall's changes, and they seem correct, at a first glance.

The only thing I could spot is the FIXME that @digitall added...

In the C++ code, we got:

ilast0 = ilast1 = 0;

but in the ARM code, we got:

lr.ilast[0] = lr.ilast[1] = 32768;

Shouldn't this be 0?

@digitall
Copy link
Member

digitall commented Oct 29, 2014

@fuzzie Could you cherry pick digitall@9003ce5 on top of your Pull Request branch and then deal with the FIXME @bluegr indicates please? :)

@bluegr
Copy link
Member

bluegr commented Oct 31, 2014

I've just talked with Robin Watts, who wrote the ARM code, and he said that @digitall's changes are OK. The different value in the ARM code was added by him in commit b9c8c6c. That value is holding a rounding bit, and he said that this could be changed to 16384, but it's way too small for anyone to hear a difference.

@digitall
Copy link
Member

digitall commented Oct 31, 2014

@bluegr : Thanks for spending the time to review this. Hopefully now fuzzie can cherry pick, deal with the FIXME and we can merge! :)

@digitall
Copy link
Member

digitall commented Jan 30, 2015

@fuzzie : What is the status of this? If you are busy, if you add me as a contributer on this branch, I will do the change myself so this can get merged.

@bluegr
Copy link
Member

bluegr commented Jun 24, 2015

Any updates on this? Can the changes from fuzzie and digitall be added?

Some feedback is needed from @fuzzie, @sev-, @wjp, @lordhoto

@sev-
Copy link
Member

sev- commented Nov 7, 2015

@digitall, could you please issue new pull request based on your branch, and we merge it and close this PR?

@digitall
Copy link
Member

digitall commented Nov 9, 2015

I have opened PR #625 to include my commit as well. The FIXME can be removed later by @bluegr or @fuzzie if they and Robin Watts are happy that the code is correct there, although it should probably just be changed to a comment to explain why it is different when compared to the C code equivalent.

@digitall
Copy link
Member

digitall commented Nov 9, 2015

@sev- : I leave whether to merge PR #625 and close this PR up to you.

@fuzzie
Copy link
Member Author

fuzzie commented Nov 9, 2015

Thanks both. We can discuss further in PR #625 if needed (but I don't think we need to).

@fuzzie fuzzie closed this Nov 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.