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

fpcalc: Use avcodec.h resampling functions for the conversion #19

Closed
wants to merge 3 commits into from

Conversation

laarmen
Copy link

@laarmen laarmen commented Dec 26, 2011

The functions used previously were internal to libav and thus the API
stability is not guaranteed between libav releases.

All the hardcoded values seem to be default values that I gathered from libavcodec source code.

Sadly, I was unable to test the code. It should work, though.

I intend to use this patch on the next version of the debian package.

Cheers,

Simon

The functions used previously were internal to libav and thus the API
stability is not guaranteed between libav releases.
@lalinsky
Copy link
Member

I'm not too happy about merging this, I'd like to see timing comparison between the current version and this one.

I can't do it myself, because the code doesn't compile for me:

Linking CXX static library libchromaprint_p.a
[ 97%] Built target chromaprint_p
[100%] Building C object examples/CMakeFiles/fpcalc.dir/fpcalc.c.o
/home/lukas/code/acoustid/chromaprint-git/examples/fpcalc.c: In function ‘decode_audio_file’:
/home/lukas/code/acoustid/chromaprint-git/examples/fpcalc.c:131:17: warning: implicit declaration of function ‘av_get_bytes_per_sample’
Linking C executable fpcalc
CMakeFiles/fpcalc.dir/fpcalc.c.o: In function `decode_audio_file':
fpcalc.c:(.text+0x2b9): undefined reference to `av_get_bytes_per_sample'
collect2: ld returned 1 exit status
make[2]: *** [examples/fpcalc] Error 1
make[1]: *** [examples/CMakeFiles/fpcalc.dir/all] Error 2
make: *** [all] Error 2
lukas@gurgle:~/code/acoustid/chromaprint-git$ 

All code in Chromaprint should compile on Ubuntu Lucid or newer, which means FFmepg 0.5.1.

@laarmen
Copy link
Author

laarmen commented Dec 26, 2011

On Mon, Dec 26, 2011 at 01:07:08PM -0800, Lukáš Lalinský wrote:

I'm not too happy about merging this, I'd like to see timing comparison between the current version and this one.
No problem, I'd be reluctant too. I'd be happy to do benchmarks and
stuff, sadly I don't have any file to test it on. I guess I could dig
into the documentation to create one by hand, but if you have one handy,
feel free to send it to me.

I'll hold off uploading the Debian package until this whole thing is
resolved one way or another.

I can't do it myself, because the code doesn't compile for me:
[...]
CMakeFiles/fpcalc.dir/fpcalc.c.o: In function decode_audio_file': fpcalc.c:(.text+0x2b9): undefined reference toav_get_bytes_per_sample'

And I thought I was being clever using this function instead of the old
av_get_bits_per_sample_format...

All code in Chromaprint should compile on Ubuntu Lucid or newer, which means FFmepg 0.5.1.

Good to know. I will set up a Lucid chroot to make it right tomorrow.

Cheers,

Simon

@laarmen
Copy link
Author

laarmen commented Dec 27, 2011

Ok, I've done some testing. You were totally right not to be happy about the patch, since it seems using the general API not only add some unnecessary overhead, but also alters the fingerprint, even when converting from s16 to s32 (which shouldn't happen since all the information is kept). I guess I'll talk to the libav folk to see if they can make public the audio resampling API.

Sorry for the fuss !

Cheers,

Simon

@laarmen laarmen closed this Dec 27, 2011
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

2 participants