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

Using TagLib for opus file tags. #9930

Merged
merged 2 commits into from Jun 12, 2016
Merged

Using TagLib for opus file tags. #9930

merged 2 commits into from Jun 12, 2016

Conversation

@beedaddy
Copy link
Contributor

beedaddy commented Jun 7, 2016

Thus gathering opus tags is on par with, for example, ogg vorbis tags.

@fritsch

This comment has been minimized.

Copy link
Member

fritsch commented Jun 7, 2016

@beedaddy which information are you missing from FFmpeg Tagloader?

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented Jun 7, 2016

@fritsch what is the FFmpeg equivalent of TagLoaderTagLib.cpp?
Does it populate MusicInfoTag for opus files with all the tag data that say FLAC files get from Vorbis comments? I have no opus files to test with, but guessing that is the reason for this PR

@beedaddy

This comment has been minimized.

Copy link
Contributor Author

beedaddy commented Jun 7, 2016

@fritsch I was wondering... Please correct me if I'm wrong but the ffmpeg based tag reader only collects title, album, artist and tracknumber. But the taglib based tag reader already collects a lot more tags. So I thought switching to taglib is only a small change and thus it's on par with other file types which use xiphcomments.

@beedaddy

This comment has been minimized.

Copy link
Contributor Author

beedaddy commented Jun 7, 2016

(But I'm very (!) new to Kodi, so I might have missed the reason why not to use the taglib approach.)

@fritsch

This comment has been minimized.

Copy link
Member

fritsch commented Jun 7, 2016

@beedaddy the ffmpeg tag reader was implemented when taglib did not yet support opus, which gave us the advantage of having metadata also when taglib has no support for it.

Do we need to bump taglib dependency for opus? Which version was the first version that supported it?

@beedaddy

This comment has been minimized.

Copy link
Contributor Author

beedaddy commented Jun 7, 2016

@fritsch I think it was version 1.9.

@fritsch

This comment has been minimized.

Copy link
Member

fritsch commented Jun 7, 2016

Yeah - that way we loose support for Ubuntu 14.04. Technically your commit is fine, but I fear we break on different platforms as kodi currently only depends on >= 1.8

@wsnipex do you have an overview where we have taglib 1.9.x ready and where not?

@beedaddy

This comment has been minimized.

Copy link
Contributor Author

beedaddy commented Jun 7, 2016

@fritsch Ah, I haven't thought about that... Perhaps because taglib 1.9 is from 2013...

@fritsch

This comment has been minimized.

Copy link
Member

fritsch commented Jun 7, 2016

@beedaddy some people run debian stable :p don't mess with them.

@beedaddy

This comment has been minimized.

Copy link
Contributor Author

beedaddy commented Jun 7, 2016

@fritsch But even they (Jessie) use 1.9.1, don't they? :)

@fritsch

This comment has been minimized.

Copy link
Member

fritsch commented Jun 7, 2016

@beedaddy it's on our chief packager to decide, you know as most likely also bumps in our depends need to happen, that's why I pinged him. Personally I also don't see an issue with bumping to 1.9.x but we need to ask before doing so.

@stefansaraev

This comment has been minimized.

Copy link
Contributor

stefansaraev commented Jun 7, 2016

taglib is already 1.11 in depends (so that's used for android / windows / osx)

we have taglib 1.9.1 in trusty and vivid stable ppas. EDIT: all ppas, unstable and nightly are also 1.9.1

@fritsch

This comment has been minimized.

Copy link
Member

fritsch commented Jun 7, 2016

configure.ac:PKG_CHECK_MODULES([TAGLIB], [taglib >= 1.8]

@fritsch

This comment has been minimized.

Copy link
Member

fritsch commented Jun 7, 2016

@stefansaraev okay, then just bumping the configure should be good enough, right?

@wsnipex

This comment has been minimized.

Copy link
Member

wsnipex commented Jun 7, 2016

yes, I backported taglib 1.9.1 to all ppas, so just bump min version in configure.ac and project/cmake/modules/FindTagLib.cmake

@fritsch

This comment has been minimized.

Copy link
Member

fritsch commented Jun 7, 2016

@beedaddy can you add an additional commit (Depends: Bump taglib min required version to 1.9) to your PR? Then it is ready to go.

@wsnipex thx for commenting.

@stefansaraev

This comment has been minimized.

Copy link
Contributor

stefansaraev commented Jun 7, 2016

jenkins build this please

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented Jun 9, 2016

Beedaddy thanks for this, I have been vaguely aware that opus wasn't getting as much tag data as othe formats but then forgot all about it.

But can anyone let me have some fully tagged opus files to test this with please?

@beedaddy

This comment has been minimized.

Copy link
Contributor Author

beedaddy commented Jun 9, 2016

@DaveTBlake You could perhaps just convert a flac file with opusenc (from opus-tools). It transfers most of the tags.

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented Jun 11, 2016

+1

Once I had an opus file I could generate more tagged variations for testing. All looks fine to me.

@stefansaraev stefansaraev merged commit 65f1dce into xbmc:master Jun 12, 2016
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.