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

fixed: broken rfft implementation used in audio visualizer api #7047

Closed
wants to merge 2 commits into from

Conversation

fritsch
Copy link
Member

@fritsch fritsch commented May 2, 2015

this replaces it with a working version using kissfft

This is a follow up of: #7039

this replaces it with a working version using kissfft
@fritsch
Copy link
Member Author

fritsch commented May 2, 2015

I would be thankful for PR of the relevant project updates we need.

@FernetMenta
Copy link
Contributor

how about moving this to depends as @wsnipex volunteered to do this?

@fritsch
Copy link
Member Author

fritsch commented May 2, 2015

All in favor for doing it, if it makes things cleaner. Thanks in advance @wsnipex

@Paxxi
Copy link
Member

Paxxi commented May 2, 2015

if we move to depends we'll have to redo the VS parts but it's simple enough so I can take care of that part

@Memphiz
Copy link
Member

Memphiz commented May 2, 2015

yes thx in advance to whoever moves it to depends :)

@wsnipex
Copy link
Member

wsnipex commented May 2, 2015

had a quick look at it and now I'm doubting the benefit of moving to depends. The lib is ~4 files, but the changes needed to move it to depends will be roughly the same :)

  1. the Makefile is useless for anything but linux and even there it needs changes for cross compiling, so we have to redo the whole buildsys
  2. needs ubuntu(and probably lots of other distros) packaging

@FernetMenta
Copy link
Contributor

maybe we should go for fftw then. this is available on the various Linux distros, and also available as pre-built for Windows.

@wsnipex
Copy link
Member

wsnipex commented May 3, 2015

I think we should just merge it as is. Its not very likely that this lib will change.

@FernetMenta
Copy link
Contributor

no need to rush. we are going to use fftw anyway in addons. there have to be a really good reason for going with kiss and breaking the strategy of getting rid of libs folder.

@fritsch
Copy link
Member Author

fritsch commented May 3, 2015

One point for kiss is: I understand the code and the algorithm it
implements, as everybody else that did fourier analysis. Furthermore it
fixes real issues we have with most of viz addons that need frequency data.
Nobody knows about side effects fftw produces on non Linux systems ...

2015-05-03 11:21 GMT+02:00 Rainer Hochecker notifications@github.com:

no need to rush. we are going to use fftw anyway in addons. there have to
be a really good reason for going with kiss and breaking the strategy of
getting rid of libs folder.

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

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

@FernetMenta
Copy link
Contributor

we could also add the sources of kiss to some "contrib" folder within the xbmc project files.

@Paxxi
Copy link
Member

Paxxi commented May 3, 2015

@FernetMenta isn't that what the lib folder basically is?

@FernetMenta
Copy link
Contributor

Nope, why crating a lib here? Those two files could be added to the project without compiling a lib and link it statically to the app. Adding an additional VS project is really bad for those 2 files.

@Paxxi
Copy link
Member

Paxxi commented May 3, 2015

They could be added into main project. i chose this method to maintain a clean separation between our code and 3rd party. The project is statically linked so end result is the same.

@AchimTuran
Copy link
Member

Nobody knows about side effects fftw produces on non Linux systems ...

On windows all works very well :)
But I'm not familiar with OSX or Android.

I will use fftw in my GSoC proposal for my xconvolver addon. So we will see how good it works.

@fritsch
Copy link
Member Author

fritsch commented May 3, 2015

http://www.fftw.org/install/windows.html <- First two sentences.

@Memphiz
Copy link
Member

Memphiz commented May 3, 2015

agree with @FernetMenta - we should add some sort of 3rdparty folder and can add those source files directly to the xcode and vs project files (and the makefiles of course). No need to tinker with libraries in that case.

@wsnipex
Copy link
Member

wsnipex commented May 4, 2015

+1 on moving it to contrib and directly using the files.

@AchimTuran
Copy link
Member

http://www.fftw.org/install/windows.html <- First two sentences.

That's true, but I think they don't wanna support Windows :)

kiss FFT Readme ;)

DO NOT:
... use Kiss if you need the Fastest Fourier Transform in the World

There is also some support for SIMD in kiss FFT.

As conclusion we need both :)
Today I started to code the benchmark. Next days I will share my results.

@Memphiz
Copy link
Member

Memphiz commented May 10, 2015

@fritsch mind if i replace this PR with this one here:

https://github.com/Memphiz/xbmc/commits/kiss_rfft

(which basically split the commits properly and moved the source to xbmc/contrib/kissfft)

Also @notspiff how can i test this - i tried all viz i have (windows and osx) but not a single one wants freqdata (so fft isn't called for any of them).

@notspiff
Copy link
Contributor

Use nastyfft from my gits.

@fritsch
Copy link
Member Author

fritsch commented May 10, 2015

@Memphiz you are welcome. THX much.
Am 10.05.2015 6:14 nachm. schrieb "Arne Morten Kvarving" <
notifications@github.com>:

Use nastyfft from my gits.

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

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

Successfully merging this pull request may close these issues.

None yet

8 participants