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

Lame VBR Preset #66

Closed
lazka opened this issue Jul 4, 2014 · 39 comments
Closed

Lame VBR Preset #66

lazka opened this issue Jul 4, 2014 · 39 comments
Labels

Comments

@lazka
Copy link
Member

@lazka lazka commented Jul 4, 2014

Originally reported by: Christoph Reiter (Bitbucket: lazka, GitHub: lazka)


From Ph0X...@gmail.com on June 03, 2010 23:04:55

I've gone through all python libraries, and even other programs and 
libraries, but I can't find anything that can figure out the VBR preset 
used by LAME on an mp3.

The only program I know of that does it is Mr. QuestionMan http://www.burrrn.net/?page_id=5 And it works very well, but I want to make renaming scripts and stuff that 
would need to parse mp3s and find that information. Sadly, I can't find any 
way of doing this.

It would be really great if you guys could implement it. Not sure why no 
one has made anything similar. If they can do it, I'm guessing it's 
possible? Unless they guess it according to the average bitrate, but I 
personally doubt that.

And I'm speaking about v0-v9 setting, of course.

Original issue: http://code.google.com/p/mutagen/issues/detail?id=66


@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From nbvF...@gmail.com on July 30, 2010 13:07:03

I have an edited version of 1.16 that does just that. Here is the file thats changed: http://pastebin.com/iLTgciTW I'd love to see this get added to mutagen, so I don't have to keep using this hacked up version...
@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From joe.wreschnig@gmail.com on July 31, 2010 01:55:43

If you submit a patch, we can look at it.
@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From brodie....@gmail.com on August 18, 2010 22:28:27

I've been sitting on my own version of this for quite some time now. I'm attaching my patch with this comment.

I see the code nbvFOUR used to guess the LAME preset is the same code that I wrote for dnuos (also used in my patch).

Let me know if you think this approach is fine. If you're happy with it, I can go ahead and write tests to accompany the changes.

Attachment: lame-support.patch

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From nbvF...@gmail.com on August 02, 2010 11:20:02

Its actually a modified off of the 1.12 release of mutagen, and I can't find that version anywhere, so I can't make a diff. I didn't do the modifications, so I can't work out what was all changed.
@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From brodie....@gmail.com on August 18, 2010 22:30:39

Whoops, here's a SVN diff if you'd prefer that (I'm using hgsubversion and mq to work on the patch).

Attachment: lame-support-svn.patch

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From the.r3m0...@gmail.com on September 13, 2010 11:07:23

Ph0b0x01, dnuos supports this as well.
@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From brodie....@gmail.com on August 18, 2010 22:49:40

Updated patch with better doc string and _guess_lame_preset moved into MPEGInfo as a method.

Attachment: lame-support-2.patch

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From hch...@gmail.com on April 11, 2013 10:59:53

I'm interested in this functionality (so that we can get info about the LAME presets in beets), so I'm going to try improving the patch for inclusion. For what it's worth, the patch applied just fine for me to present cloned master code!
@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From joe.wreschnig@gmail.com on April 17, 2011 05:14:54

Two notes - 

1. Please wrap all the unpack calls calls (and anything else that can throw an exception) some try/except wrappers. Otherwise parse errors in the LAME data (I don't know how common this is) will result in files that were previously readable becoming unreadable.

2. The logic in the "elif version >= (3, 90) and version < (3, 97):" case looks like it might be more suited for a dictionary or multidimensional dictionary than a nested conditional.

Otherwise, this looks good for inclusion once it has tests.

Labels: NeedsTest

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From hch...@gmail.com on April 12, 2013 10:17:40

I've added some try/except statements and a potential dictionary solution to the nested conditional you've mentioned... I'm interested in writing some tests, but none of the present MP3s in tests/data have LAME data. Are there any good guides to making tests, or at least data for these tests, for mutagen? I suppose I will have to get old versions of LAME and encode files with those as well?

Anyways, I've attached my altered patch. (I've never made patches like this before, so pardon if it's more trouble to work with than it should be)

Attachment: mp3_lame_headers.patch

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jul 4, 2014

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


From hch...@gmail.com on April 15, 2013 09:17:59

I've added testing for the lame preset calculations into test_mp3.py, using the test data from dnuos. I'm glad to help alter/fix this further for any functional/legal/stylistic reasons. I've put my changes over in my mercurial fork https://bitbucket.org/hchapman/mutagen .
@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Aug 11, 2015

Original comment by Arturo R. (Bitbucket: jaquer, GitHub: jaquer):


Giving this a little bump. @hchapman seems to have done all the work over at his forked repo, and it used to apply almost cleanly to the upstream tip, at least to my novice eyes. I'll be happy to fork and cleanup for a proper PR.

I've been successfully using his version for a while now, but would love to see it available at the main distribution so I can use it with downstream projects (e.g. beets).

edit: Doh! That's what I get for not checking new commits before commenting. Looks like some refactoring of the Xing parsing took place, and the code is out of sync now. I'd love to help out in any way possible to bring it up to par.

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Aug 11, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


Yeah, bad timing. But on the other hand I'm feeling more positive about including something like this now, as things are cleaned up a bit.

What's the use case for this?

I'm wondering if it would be enough for starters if we expose a "encoder_info" text only attribute without a specified format similar to "Mr. QuestionMan"

e.g. encoder_info="LAME 3.91 [alt-]preset extreme -b 192"

or in case a VBRI header is found

encoder_info="FHG"...

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Aug 11, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


I've created a test vector for all lame versions: https://www.dropbox.com/s/u487gyf36mkocwg/lame_versions.tar.xz?dl=0

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Aug 11, 2015

Original comment by Arturo R. (Bitbucket: jaquer, GitHub: jaquer):


Thanks for looking into it. For my particular use case, a text string is plenty. I can parse it on my end.

Here's Harrison's sample code that uses the text returned from his lame_tag parsing: https://github.com/hchapman/beets_encoding_plugin/blob/master/beetsplug/qualitytrump.py#L29-L36

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Aug 11, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


mp3: parse lame headers. See #66

This parses the version and the header but the result does
not get exposed for now.

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Aug 11, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


There is now some code in trunk to parse everything. But it's only used for the length calculation for now.

Thanks for looking into it. For my particular use case, a text string is plenty. I can parse it on my end.

That wont work as the format wouldn't be stable, the only thing you can do is display it to the user as is.

I'll have a look at the plugin/use case..

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Aug 12, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


On the original feature request: If detecting mp3 encoders like Mr. QuestionMan looks like described here http://www.hydrogenaud.io/forums/index.php?showtopic=11785, there is no way that's worth the added complexity (also given the uncertainty of the result). Especially since nowadays all you want to know is LAME nor NOT-LAME.

For the beets plugin use case, comparing two encodings of the same source by unknown encoders/settings: Extracting a good guess of the passed settings might be possible, but it's not even clear how to compare them and how the result changed with different lame versions. If you compare all the -V/--preset settings for newer LAME the easiest value to compare for the same source is just the bitrate.

I propose exposing the following:

encoder_info = "LAME 3.XX" or "" # where lame encoded files are specified to start with "LAME" here, the rest is undefined.

bitrate_mode = VBR or ABR or CBR or UNKNOWN (probably CBR but not sure)

bitrate = same as now

So you can show "LAME 3.99, CBR, 192kpbs" vs "LAME 3.99, VBR, 165kbps" and for programmatically detecting take the bitrate plus add e.g. +20% for VBR, +15% for ABR, +5% for UNKNOWN and +10% for LAME and compare the result.

Any thoughts?

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Aug 13, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


The above is now in trunk.

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Aug 13, 2015

Original comment by Arturo R. (Bitbucket: jaquer, GitHub: jaquer):


Thank you so much for the work on it Christoph. I think that now that the LAME header is its own class, it's easier to extend it if so desired.

This is a bit above my skill level, so I'll ping Harrison again, see if he has any further input.

Thanks again.

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Aug 13, 2015

Original comment by Harrison Chapman (Bitbucket: hchapman, GitHub: hchapman):


Hey! I'm really excited to see that this seems to be moving along. I haven't looked at this in years (woah!) but I can try my best if it will help this feature get added.

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Nov 30, 2015

Original comment by Thomas Ruddell (Bitbucket: tthomass, GitHub: tthomass):


Can stereo separation be exposed also?
Mid/side/jstereo/stereo/separate stereo, or whatever is best.

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Nov 30, 2015

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):


It already is.

lazka added a commit that referenced this issue Apr 7, 2016
This parses the version and the header but the result does
not get exposed for now.
@lazka lazka removed the trivial label Apr 27, 2016
@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jun 25, 2016

test vector on bitbucket, so it doesn't get lost: https://github.com/lazka/mutagen-mp3-test-vector

The beets issue, which was never directly referenced here afaics: beetbox/beets#116

Closing this. If anyone has a use case/interface proposal to expose more, please bring it up here.

@mikeacameron

This comment has been minimized.

Copy link

@mikeacameron mikeacameron commented Jan 18, 2017

I don't know if you can and/or want to comment on the referenced issue above?

My use case is that when using beets, it would be nice to be able to query my library for specific VBR quality. Looking at the average bitrate of a file, it rarely seems to match what the LAME targets propose from here.

Right now I just have a barebones version of parts of dnuos that I converted to Python 3 in order to get the LAME codec profile.

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jan 18, 2017

Do you indent to use the extracted quality somehow or do you just want to know it (for display purposes..)?

@sampsyo

This comment has been minimized.

Copy link
Contributor

@sampsyo sampsyo commented Jan 18, 2017

@lengtche, I notice that LAMEHeader already has a preset_used field. Does that contain what you need?

@mikeacameron

This comment has been minimized.

Copy link

@mikeacameron mikeacameron commented Jan 18, 2017

@lazka It's for a) display purposes on my folder names and b) querying in beets with beets ls -a albumquality:v0. I think I remember seeing a commit in this repo where the information was exposed, but I can't find it anymore.

@sampsyo Think I remember preset_used only returned whether it was VBR or not. It's been a few weeks since I checked.

@mikeacameron

This comment has been minimized.

Copy link

@mikeacameron mikeacameron commented Jan 18, 2017

I see class LAMEHeader that @sampsyo is mentioning. Searching the repository only seems to indicate that it's not yet available for consumption. In this class there's property vbr_quality, which looks like what I want.

@sampsyo

This comment has been minimized.

Copy link
Contributor

@sampsyo sampsyo commented Jan 19, 2017

Indeed! I did a little poking, and it's possible to construct the right object:

>>> mf = mutagen.File(fn)
>>> offset = mutagen.mp3._util.XingHeader.get_offset(mf.info)
>>> f = open(fn)
>>> f.seek(mf.info.frame_offset + offset, 0)
>>> xh = mutagen.mp3._util.XingHeader(f)
>>> xh.lame_header.vbr_quality
0
>>> xh.lame_header.stereo_mode
3

That does require opening the file again, though. Maybe the MPEGFrame object should keep around the XingHeader that's parsed, if any.

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jan 19, 2017

What about

mutagen.mp3.MPEGInfo.encoder_settings
"""text: a guess about the settings used for encoding, e.g. "-V0" for LAME.
Values might change in the future, use for display purposes only.
"""
@lazka lazka reopened this Jan 19, 2017
@mikeacameron

This comment has been minimized.

Copy link

@mikeacameron mikeacameron commented Jan 19, 2017

@sampsyo Thank you. I tried tinkering with _util before but didn't figure it out.

@lazka That would work. I assume it wouldn't simply return the id3v2's TSSE heading, right?

@sampsyo

This comment has been minimized.

Copy link
Contributor

@sampsyo sampsyo commented Jan 19, 2017

fn is just a filename.

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jan 20, 2017

@lengtche yes

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jan 20, 2017

Here is what I get with the dnuos logic: https://bpaste.net/show/a730d569e9fd

a bit hit and miss

lazka added a commit that referenced this issue Jan 20, 2017
@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jan 20, 2017

@sampsyo

This comment has been minimized.

Copy link
Contributor

@sampsyo sampsyo commented Jan 21, 2017

Wow; that looks quite accurate! Nice work on the heuristic. Does this look useful for your purposes, @lengtche?

@mikeacameron

This comment has been minimized.

Copy link

@mikeacameron mikeacameron commented Jan 21, 2017

Oh, woah! This is fantastic.

That is exactly what I was looking for.

Much appreciated.

@lazka

This comment has been minimized.

Copy link
Member Author

@lazka lazka commented Jan 21, 2017

Ok.

@lazka lazka closed this Jan 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.