Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Drop MP3Codec/libmad from paplayer and dvdplayer in favor of FFmpeg #4499

Merged
merged 10 commits into from

10 participants

@herrnst
Collaborator

Follow-up to #4386 and initially started by @fritsch - this drops libmad and MP3Codec stuff from paplayer and dvdplayer and rather utilises ffmpeg. While this removes a rather dated dependency (libmad was last updated 2004), the mpg123 decoder inside ffmpeg improves playback of MP3 audio files not being 100% compliant and might play back (partially) distorted when using libmad.

Linux should be fine. OSX (Xcode 5.1) 32bit build tested and working. Win32 project changes need review and testing. Other platforms need to be tested, too.

Probably only for G+1.

Credits belong to @fritsch who started the work :)

@t-nelson

Any complaints about reordering the commits so all of the build system stuff is first? That way every commit should compile, which may ease a future bisect.

@t-nelson t-nelson added the Helix label
@jmarshallnz
Owner

Personally I'd prefer that we aim for this to be moved to an add-on rather than completely removed.

Ofcourse, the switch to ffmpeg could happen sooner if there's really a bunch of non-compliant files out there (I haven't come across any, but don't use weird encoders), but I'd prefer to leave the support for libmad (ifdef'd out as required) as an incentive for bin add-ons.

IIRC @opdenkamp has some work on audio codecs, as well as ofcourse spiff.

@davilla

I'm for making a dependency die off.

@t-nelson
@jmarshallnz
Owner

The work has already been done. So it's a matter of annoying folk until it's merged.

@wsoltys
Collaborator

Does ffmpeg need any external lib for this? If yes we would have to add it to our build script and to our mingw environment.

@herrnst
Collaborator

@t-nelson Project/depend update commits moved to the top.

@jmarshallnz Probably there are not many files causing issues, but I have at least one candidate that does a garbling sound at one position, which isn't there when playing back with mpg123/ffmpeg (or even tools like winamp). Also - which honestly is a really "synthetic" case - if you use a tool like MP3Gain to increase the gain of a file and over-amplify to like +20db, libmad will produce really screetchy clipping noise, while anything other will clip.

@wsoltys Looking at ffmpeg.git/libavcodec/mpegaudio*, everythings there, but someone with proper ffmpeg knowledge should confirm this ;)

@fritsch
Collaborator

@wsnipex Could you have a look if our ffmpeg is ready? I tested it with ffmpeg 2.2.x and here everything worked - but not sure if it's build especially cause of some -dev packages I have arround

@opdenkamp
Collaborator

@jmarshallnz re audio bin codec add-on: correct, i'll clean it up and create PRs soon.

@wsnipex
Collaborator

configure.in needs cleanup as well. I wonder if we should remove the lame config option and switch cdrip over to ffmpeg as well.

@fritsch we use the default mpegaudio codec in ffmpeg2 and it just works.

@herrnst
Collaborator

Appended some more smaller commits which need squashing:

docs/README* should also be cleaned up (also still mentions libflac stuff which is also gone already), but IMHO that's stuff for another PR.

@herrnst
Collaborator

FWIW: Tested this on Linux with the FFmpeg 2.2 stuff. Noticed that a few frames are missing at the beginning of every file (even after trying to rewind to 00:00:00), but besides that everything works flawless.

Also squashed the additional commits as mentioned above a few days ago.

@FernetMenta
Collaborator

why does rewind to 0 not work? would it make any difference if you set the backwards flag to true?
https://github.com/xbmc/xbmc/blob/master/xbmc/cores/paplayer/DVDPlayerCodec.cpp#L266

@herrnst
Collaborator

Yep, that makes a difference (in fact, files are now played back complete without a larger number of missing frames at the beginning) - commit at herrnst@39e323f

These SeekTime lines gets pushed to xbmc.log before this (when rewinding to zero, 2940 being the last message, smells alot like the offset):

13:02:41 T:140626569565952   DEBUG: SeekTime - seek ended up on time 32600
13:02:41 T:140626569565952   DEBUG: SeekTime - seek ended up on time 2940

after (and without missing audio):

13:04:20 T:140395283470080   DEBUG: SeekTime - seek ended up on time 35566
13:04:21 T:140395283470080   DEBUG: SeekTime - seek ended up on time 5094
13:04:21 T:140395283470080   DEBUG: SeekTime - seek ended up on time 0

Looking at dvdplayer/DVDDemuxers/*.cpp, only the PVR demuxer actually makes use of the flag but that shouldn't be used from within paplayer. FLAC (now also handled by FFmpeg) still seeks fine, can't tell about the other DVDPlayerCodec/FFmpeg formats ATM.

EDIT: mentioned commit appended to this PR/branch.

@herrnst
Collaborator

PR updated: Last two commits make the backwards flag in ::Seek() being set properly (fixing missing frames), and enables accurate seeking in VBR mp3 files, most notable when playing MP3/CUE files.

@herrnst
Collaborator

Rebased onto master after ffmpeg bump (conflict in DVDDemuxFFmpeg resolved).

@FernetMenta
Collaborator

jenkins build this please

still angry? (obviously, grrr)

@MartijnKaijser

jenkins build this please

@herrnst
Collaborator

Hm...

Fetching upstream changes from git://github.com/xbmc/xbmc.git
FATAL: Failed to fetch from git://github.com/xbmc/xbmc.git

... since jenkins doesn't listen to me, someone mind giving him a push again?

@wsnipex
Collaborator

jenkins build this please

@FernetMenta FernetMenta merged commit 4060c71 into xbmc:master

1 check passed

Details default Merged build #558 succeeded in 1 hr 35 min
@MartijnKaijser MartijnKaijser added this to the Helix 14.0-alpha1 milestone
@herrnst herrnst deleted the herrnst:remove-mp3codec branch
@BlackIkeEagle BlackIkeEagle referenced this pull request from a commit in herecura-archived/herecura
@BlackIkeEagle BlackIkeEagle xbmc-git :: libmad is not a dependency anymore
see: xbmc/xbmc#4499

Signed-off-by: BlackEagle <ike.devolder@gmail.com>
1a067a5
@FernetMenta
Collaborator

@herrnst looks like gapless is somehow broken. the previous and next track do not fit exactly together. the old code ignored the first 528 samples. do you think we need to do the same with dvdplayer codec for mp3?

Collaborator

@FernetMenta I quick-checked with an official Helix 14.0 build on win32, there indeed is a small gap/audible sort-of blip between track changes even with MP3 files that contain zero-padding information (e.g. live-album ripped to separate tracks, play "real-gapless" in winamp). However, the same audible glitch is also there with FLAC files (which due to their bit-identical lossless compression do not suffer from gap problems by nature), so I suspect something in paplayer. I remember a longer time ago we had a talk about x-fade also having some blip upon end of fading, might be related?

Collaborator

Oh, addendum: "Play with -> dvdplayer", no audible problems.

Collaborator

eh? dvdplayer does not support gapless at all
there is an issue with mp3 and gapless because currently we don not consider decoder delays

Collaborator

Well that's what I found out "as a user". paplayer = blip with both FLAC and MP3 (of course fine with e.g. WinAmp), dvdplayer = no blip, all fine, regardless of the file format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.