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

High quality resampler #1629

Merged
merged 1 commit into from Feb 23, 2015
Merged

High quality resampler #1629

merged 1 commit into from Feb 23, 2015

Conversation

kamiyo
Copy link
Contributor

@kamiyo kamiyo commented Dec 1, 2014

Think of this as rough draft. Taking suggestions to make it robust, error free, and prettier!

For more information:
https://docs.google.com/document/d/1tBEgsJh7QiwNwepXI0eobfK3U8LkJButSyeuFt1degM/edit?usp=sharing

removed: SSE includes

added: 16bit -> float -> 16bit functions
added: linear interpolator and high-quality interpolator functions (including Resampler class)
added: dithering

changed: renamed variables and reformatted a few things to fit with style guide
changed: use s16, u16, s32, u32 etc
changed: store samples and do all computations as floats
changed: volume from 0 - 255

void CMixer::MixerFifo::PushSamples(const short *samples, unsigned int num_samples)
{
void CMixer::MixerFifo::PushSamples(const s16 *samples, u32 num_samples)
{

This comment was marked as off-topic.

@lioncash
Copy link
Member

lioncash commented Dec 2, 2014

@dolphin-emu-bot rebuild

CMixer* m_mixer;
u32 m_input_sample_rate;

std::vector<float> m_float_buffer; // [MAX_SAMPLES * 2];

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@kamiyo
Copy link
Contributor Author

kamiyo commented Dec 2, 2014

ugh M_PI doesn't work like this with in VS2013. Anyone know what the problem is?

@PatrickFerry
Copy link
Contributor

There's a good block of code that's the same between Mix() and MixLinear() can this be avoided?

@skidau
Copy link
Contributor

skidau commented Dec 4, 2014

@dolphin-emu-bot rebuild

#if _M_SSE >= 0x301 && !(defined __GNUC__ && !defined __SSSE3__)
#include <tmmintrin.h>
#ifndef M_PI
#define M_PI 3.14159265358979323846

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@skidau
Copy link
Contributor

skidau commented Dec 4, 2014

@dolphin-emu-bot rebuild

@PatrickFerry
Copy link
Contributor

Is the build issues related to the discussion on Includes?

Edit: You've added an interpolator so does this replace your other PR?

@kamiyo
Copy link
Contributor Author

kamiyo commented Dec 4, 2014

Build issue: No I think it's because of something in an unrelated file. I had fixed Speaker.cpp in another PR (#1626), and I undid that for this commit (before it was merged), and it's causing this to be all screwy. The only way I can think of is closing this and doing a clean PR. advise.

Previous interpolator: yes sorry forgot about that one. I'll close that one.

@PatrickFerry
Copy link
Contributor

You wouldn't have to close this. You could rebase then I guess.
Someone else can advise if that doesn't work. I don't foresee any issues though.

@kamiyo
Copy link
Contributor Author

kamiyo commented Dec 4, 2014

I'm trying to rebase in my gitext but it's actually giving me the same error as the build-bot.

@kamiyo
Copy link
Contributor Author

kamiyo commented Dec 4, 2014

dunno if what I did fixed it but we can try again.

@skidau
Copy link
Contributor

skidau commented Dec 7, 2014

@dolphin-emu-bot rebuild

@PatrickFerry
Copy link
Contributor

@kamiyo I think you should get rid of the three merges. That will mess things up I think

@kamiyo
Copy link
Contributor Author

kamiyo commented Dec 8, 2014

um. sorry how do I do that? I'm still very newb at git.

@kamiyo
Copy link
Contributor Author

kamiyo commented Dec 8, 2014

lol I really screwed it up. sigh.

@PatrickFerry
Copy link
Contributor

It's fine now.

@PatrickFerry
Copy link
Contributor

Delete the moved stuff around commit, easy.

@magumagu
Copy link
Contributor

magumagu commented Dec 8, 2014

@kamiyo Making a new PR doesn't do anything; the PR is just a reflection of the branch kamiyo:FIR-resampler, and you can always just force-push a new branch on top of it (git push -f).

@kamiyo
Copy link
Contributor Author

kamiyo commented Dec 8, 2014

@magumagu Ah okay. Makes sense. I think I managed to remove that bad commit though. maybe...

standby.

@kamiyo
Copy link
Contributor Author

kamiyo commented Dec 8, 2014

regarding the M_PI issue, I think we should fix that in another PR, I'll have to hunt for all of the math includes and replace them like @shuffle2 suggested.

@Buddybenj
Copy link
Contributor

Bump. Any update on this?

@kamiyo
Copy link
Contributor Author

kamiyo commented Jan 1, 2015

I am (still) all ears for improvements and such!

class LinearMixerFifo : public MixerFifo {
public:
LinearMixerFifo(CMixer* mixer, u32 sample_rate) : MixerFifo(mixer, sample_rate) {}
void Interpolate(u32 left_input_index, float* left_output, float* right_output);

This comment was marked as off-topic.

@delroth
Copy link
Member

delroth commented Jan 1, 2015

Fix these few remaining style issues and I think this can be merged. Thanks!

@delroth
Copy link
Member

delroth commented Jan 1, 2015

Oh, and clean your commit history. It's really dirty right now. Squash merge into the commit introducing the change, and reword it so that it explains what the change does better.

@oddMLan
Copy link

oddMLan commented Jan 15, 2015

@kamiyo
Bump. Did you manage to solve the remaining issues? I don't see 92 file changes anymore.

@Buddybenj
Copy link
Contributor

Do you mind rewording the commit message to be descriptive and rebasing this PR?

@kamiyo
Copy link
Contributor Author

kamiyo commented Feb 6, 2015

What do you mean by descriptive? I'll try rebasing again.

edit: I think I get what you mean. Will add info.

On Fri Feb 06 2015 at 5:44:28 PM Benjamin Przybocki <
notifications@github.com> wrote:

Do you mind rewording the commit message to be descriptive and rebasing
this PR?


Reply to this email directly or view it on GitHub
#1629 (comment).

@Buddybenj
Copy link
Contributor

The commit just says "For more information:" how about "High Quality Resampler"

@kamiyo
Copy link
Contributor Author

kamiyo commented Feb 6, 2015

haha I guess the comments I wrote under it didn't show up? I'll try to fix that and add even more =)

// linear interpolate between current and next sample
// increment output sample position
// increment input sample position by ratio, store fraction
// QUESTION: do we need to check for NUM_CROSSINGS samples before we interpolate?

This comment was marked as off-topic.

This comment was marked as off-topic.

@ADormant
Copy link

What is the status on this?

@kamiyo
Copy link
Contributor Author

kamiyo commented Feb 18, 2015

Mostly done on my end, I think.

@delroth
Copy link
Member

delroth commented Feb 18, 2015

It needs a rebase before it can be merged.

For more information:
https://docs.google.com/document/d/1tBEgsJh7QiwNwepXI0eobfK3U8LkJButSyeuFt1degM/edit?usp=sharing

removed: SSE includes (not used)

added: 16bit -> float -> 16bit functions
added: linear interpolator and high-quality (windowed-sinc) interpolator functions (including Resampler class)
added: dithering

changed: renamed variables and reformatted a few things to fit with style guide (braces, #include->const)
changed: use s16, u16, s32, u32 etc
changed: store samples and do all computations as floats
changed: volume from 0 - 255
@ADormant
Copy link

Can it be merged now?

skidau added a commit that referenced this pull request Feb 23, 2015
@skidau skidau merged commit 81eb9bd into dolphin-emu:master Feb 23, 2015
float l_output = m_output_buffer[i + 1];
float r_output = m_output_buffer[i];
TriangleDither(&m_output_buffer[i + 1], &m_output_buffer[i]);

This comment was marked as off-topic.

This comment was marked as off-topic.

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