Python version.
Closed this issue · 258 comments
This would make it more portable.
@WhitePeter I'm on the part where I need to scan directories, python's glob in 3.5+ can be recursive, but I believe most distros still use 2.7 as default, should we go for 2.7 support (which would make this code a bit more complex).
Hmm, I only have 3.4.2. ATM. This is plain old stable Debian. Of course, there is 2.7 as well. So, I'll say 2.7. ;) Or, maybe there are alternatives that run in both versions?
OTOH, why use 2.7 if you need not? It is supposed to get phased out. Does it have to be 3.5+ or would it work with older python 3 as well?
It seems the recursive glob is only in 3.5+ (pycharm doesn't even pick it up since it has 3.4 headers, even though I'm using 3.5).
Changed in version 3.5: Support for recursive globs using “**”.
https://docs.python.org/3/library/glob.html#glob.glob
It's possible to recurse in < 3.5 but it requires importing more stuff, but I think it's the better way to go, since 3.5 is maybe not available on all distros.
What about this: https://stackoverflow.com/questions/18394147/recursive-sub-folder-search-and-return-files-in-a-list-python#18394205
Doesn't look too complex and needs no recursiveness. Looks like the more concise version of the 2.2 through 3.4 example from your link. I like generators. ;)
Seems to ignore some folders, I'll upload what I have so you can test.
The 2.2 to 3.4 one from here https://stackoverflow.com/questions/2186525/use-a-glob-to-find-files-recursively-in-python/2186565#2186565 seems to work ok, the one from your link doesn't work if the file has brackets in the name.
Which kind of brackets? I always get confused by terms like this, not a native speaker. For me, brackets is anything like (), [], {}.
Sorry, I meant []
Um, BTW, should this perhaps be put in a separate branch? Just while it is still experimental. The README only talks about a shell script, for example.
Yeah, that would be a good idea!
I'm going to head to bed, if you want to muck around with the python script go ahead :)
LOL, I was just about to write something similar. Rather tired myself. Let's call it a night.
Alright, take care.
You too.
Just added my first commit in the python branch. Thought it a good thing to get warmed up.
But I think, in the long run, this should maybe change, so parameters are allowed. Most programs I came across, actually quit with a short usage info. I think that might be a good idea.
On the parameter front, I was thinking things like "treat directories as albums" and/or a simple "treat all files as on album". I think that makes sense for video content as well, think TV series.
Just thought I'd mention this now. I have never actually done anything that accepts parameters. Maybe, this would be a pain to implement if done to late in the process?
Yeah that's what I was thinking of last night when logging off.
FYI, working on making the fnmatch case-insensitive. But that is the easy part. Making it match *.[mM][kK][aAvV] or *.[mM][kK]3[dD] is the hard part. Don't say regex, that's cheating. :P
Think, I've got it. Maybe not the most elegant way to do it, but it works.
Nice, I was trying to figure that out last night.
I got the arg parser working, will push that after I merge your changes.
Works:
No path(s) given, processing current working directory recursively.
./x.mka
File is not matroska.
./x.mkv
File is not matroska.
./x.mk3d
File is not matroska.
https://pypi.python.org/pypi/enzyme/
That outputs the tags, with the UID.
Ah, now I see. We only need the UID, the RG and the original tags, right? mkvpropedit supports the uid in the track selector. Let me see, if I can get something meaningful together.
I'm not sure, it's a bit complex, because enzyme is not giving the uid with track info, only in the tags, but mkvinfo does, so there must be a way to edit enzyme to give the info.
A, crap, I forgot that we actually have to tell bs1770gain the track ID and not the uid.
https://i.imgur.com/icN22qh.png
On the top right, it's the output of enzyme, the track information first, then the tags information.
On the bottom is the output of mkvinfo.
See how mkvinfo has both the number and the uid, but enzeme only has 1 or the other.
Or is there maybe a way with enzyme?
class enzyme.mkv.Track(type=None, number=None, name=None, language=None, enabled=None, default=None, forced=None, lacing=None, codec_id=None, codec_name=None)
type=Audio, number=$(what bs1770 wants)?: So type + number should be the track ID for bs1770gain?
Yeah, we can use the track number from mkv.audio_tracks[track].number for bsgain1770, although the issue is trying to merge the tags.
I'll push what I have so you can do some testing on enzyme
Sorry, I am useless ATM. I guess I'll enjoy watching an mkvrg prepped movie. I will look at this tomorrow with fresh eyes.
Sure, no problem, just trying to get the same functionality as the bash script right now.
Just a quick FYI. Read your comment: 'enzyme returns the "track number" like mkvinfo does, but bs1770gain requires the "track ID"'
The "track number" output by mkvinfo should always be bs1770gain's track_ID+1 (the --audio option, tight?). As I understand it, what bs1770gain wants and outputs with -l, is the stream index, which starts at 0. track numbers in matroska files, however, start at 1 and are just incremental, so the last track number actually matches the total number of tracks contained. for bs1770gain, or ffprobe, the highest stream index #0:<index> is the total number of streams/tracks -1. I think that is a safe assumption, might even be documented somewhere. But the track UID can be anything, I think, so long as it is unique within the mkv.
I haven't really examined the code, yet, no time for that, right now. So, I might have misunderstood. Just wanted to share my (quick) thoughts.
Edit: And actually I'd like to know, why bs1770gain wants a stream index. It'd be so much easier to just ffmpeg -i input -f wav -map a:<audio_index> - | sox .... Why is that --video option there, in the first place? It's not like one can bs1770gain video streams, anyway. ;)
BTW, does mkvinfo --gui-mode work for you? For me, it is just the same output as the normal mode, whereas it should be program friendly, like: #GUI#message#key1=value1#key2=value2.... I thought about exploiting that to get a mapping track_num<->track_UID.
The thing that throws me off is test5.mkv from the mkv test files.
Up to track number 6, it indeed is track number minus 1 = track id, but after that, it is minus 2, because track number 7 is at the end (there are 11 track numbers).
So if we use enzyme and do track number - 1, in the case of test5.mkv the 8th track is also an audio track and the track id is 6, we would miss an audio track.
I found the location where mkvinfo calculates the track id, but the code is quite complex / hard to understand:
https://github.com/mbunkus/mkvtoolnix/blob/master/src/info/mkvinfo.cpp#L903
mkvinfo --gui-mode outputs the same information as mkvinfo for me.
mkvinfo --gui-mode outputs the same information as mkvinfo for me.
OK, was about to contact Moritz Bunkus on IRC to ask about that. Just double-checked, the current man page does have the option mention, --help option doesn't.
Good you checked the track ID thing. Downloading the test files now. ;)
Thinking out loud about the bs1770gain --video option:
What if a matroska has 2 video streams in it, both have their own audio files, would it be Stream #0:0 for the first video, and Stream #1:0 for the second video?
But bs1770gain does say:
--video <index>: select video index (corresponds to [0:<index>] in FFmpeg listing, cf. -l/--list option)
No. the first index in #<index>:<index> is the input file index. It is used for identifying which input file shall be mapped to which output file, for example. At least, that is how I remember it. I will double-check that later.
But there is good news. I got a reply on IRC. Apparently, the mkvinfo --gui-mode is more of an accident. But there is however another way:
mkvmerge --identify --identification-format json matroska_file.mkv
Looking at the bs1770gain source, the --video option seems kind of useless:
It finds the "vi" (video index) but only returns the audio index.
"mkvmerge --identify --identification-format json matroska_file.mkv"
Would be fun if we could restrict it to audio tracks only.
We can also do this:
mediainfo test5.mkv --Inform="Audio;%ID%\n"
(then we get the bs1770gain style ID's)
Although I like the mkvmerge --identify --identification-format json matroska_file.mkv better since it can mean we can get the tags, since it has the UID's
Huh, haven't looked at it that way. Good idea.
Have to head out, will be back this afternoon, thanks for the info.
You're very welcome. :)
But bs1770gain does say:
--video <index>: select video index (corresponds to [0:<index>] in FFmpeg listing, cf. -l/--list option)
Yes, that is what I actually meant. But note that there is only one <index>, and the first index seems hardcoded to 0. Here is an example output off ffmpeg with two input files, since ffprobe only supports one file:
[...] # 1st input file, leftmost index is 0
Stream #0:0: Video: h264 (Main), yuv420p(tv, smpte170m/smpte170m/bt709), 1024x576, SAR 1:1 DAR 16:9, 24 fps, 24 tbr, 1k tbn, 2k tbc (default)
Stream #0:1: Audio: aac (LC), 48000 Hz, stereo, fltp (default)
Stream #0:2(eng): Subtitle: subrip (default)
[...] # 2nd file leftmost index is now 1, or more precisely #1
Stream #1:0: Video: h264 (Main), yuv420p(tv, smpte170m/smpte170m/bt709), 1024x576, SAR 1:1 DAR 16:9, 24 fps, 24 tbr, 1k tbn, 2k tbc (default)
Stream #1:1: Audio: aac (LC), 48000 Hz, stereo, fltp (default)
Stream #1:2(eng): Subtitle: subrip (default)
An actual command would look like this:
ffmpeg -i file0 -i file1 -c copy -map 0 -map 1 single_ouputfile
This way one can merge (not concatenate!) more than one file into a single one. By default, without -map there would have to be one output file per input file.
I still think using this kind of "Stream Specifier", as they are called in ffmpeg is a suboptimal choice in bs1770gain, since one can also use things like -map a:0 or -map 0:a or even -map a, which only selects audio tracks, like 1st audio track from all inputs, all audio tracks from 1st input or all audio tracks, respectively.
But then the audio index starts with zero again, no matter which #<file_index>:<stream_index> the first audio stream has. And the highest audio index is num_total_audio-1. It is quite handy, actually, once one gets used to it.
Anyway, for us this might be a lucky coincidence, because mkvmerge -i ouputs the "Track ID" in the exact same order and numbering scheme as ffprobe/ffmpeg, and hence bs1770gain, does. See this for test5.mkv:
% mkvmerge -i test5.mkv # chose the short form for readability, json would just clutter this up.
File 'test5.mkv': container: Matroska
Track ID 0: video (MPEG-4p10/AVC/h.264)
Track ID 1: audio (AAC)
[...] # leaving out the subs
Track ID 8: audio (AAC)
[...]
% ffprobe test5.mkv
[...]
Stream #0:0: Video: h264 (Main), yuv420p(tv, smpte170m/smpte170m/bt709), 1024x576, SAR 1:1 DAR 16:9, 24 fps, 24 tbr, 1k tbn, 2k tbc (default)
Stream #0:1: Audio: aac (LC), 48000 Hz, stereo, fltp (default)
[...] # again, no subs, not important here.
Stream #0:8(eng): Audio: aac (LC), 22050 Hz, mono, fltpSo, the track/stream index you feed to bs1770gain can be reused for mkvmerge or mkvpropedit. As mkvrg works now, it selects by track type audio. I think, it would be much easier, to just say:
mkvpropedit --tags all:tags_ammended_with_RG.xml $file
Or, we could make that:
mkvpropedit --tags track:track_ID:tags_ammended_with_RG.xml $file
But I would prefer the first version, since that would be less code I reckon. In the second case we'd also have to strip the original tags that are unrelated to the tracks we manipulate. In the first we'd only have to add the RG tags.
And since we get the original tags in json format, it should be much easier to manipulate them. Not that I know anything about that, though. But I think I can do that, once I learn about it.
P.S.: We might need to keep an I on bs1770gain, in case the developers also get the idea to use better stream specifiers. ;) Could make all this fall apart. But that --video has got to go, it's just silly. :P
P.P.S: I guess this means we can drop enzyme, doesn't it?
I am going to have a look at the code now, to see if I can get some proof of concept together, that does not need enzyme. mkvtoolnix and bs1770gain should be enough.
P.S.: Ping me, when you get back and want to have a look yourself, because this might take me quite some time. You are way, way more proficient than I am. :)
Made some changes to mkvrg to make my point in the above comment clearer. Now it works without $trackindex, simply using $track, which came from bs1770gain. I checked with test5.mkv and it works. :)
What I learned in the process: mkvpropedit --tags track:(track_id+1):foo.xml bar.mkv is the way to go. This is documented behaviour in mkvpropedit.
I think that is all the more reason to just extract the tags, manipulate the ones relevant to audio and then write the whole thing back. No headaches with track numbering in mkvpropedit this way.
Nice find with the mkvpropedit --tags track:(track_id+1):foo.xml bar.mkv
Will get back to it in a few.
FYI, just discovered that one can quickly wipe all tags by mkvpropedit --tags all:/dev/null matroska_file.mkv
Might come in handy, i.e. to see if the new iteration of mkvrg.py actually writes tags and at the proper positions. It's good to start with a clean slate there.
I've finished porting the bash script to python, so now we would need to figure out how to do the hard part, merging the XML's and stuff.
FYI, just discovered that one can quickly wipe all tags by mkvpropedit --tags all:/dev/null matroska_file.mkv
Might come in handy, i.e. to see if the new iteration of mkvrg.py actually writes tags and at the proper positions. It's good to start with a clean slate there.
I think your (trackid + 1) idea is working good, I've tested on a few files.
Phew, it took me longer than expected to get around to actually doing as I said, and checking the code. Some lines are way, way too long for my measly 19" monitor. ;)
What about it, I go and make sure the code is pep8 compliant? Still have some checkers for that in my editor.
And I think there is some redundancy in there, i.e. checking if the file is a matroska, we don't need file magic for that. Any of the mkvtoolnix will shout error loud enough. :) Also, the commands which and file are most likely not present in Windows. I just removed "which" from the required list. Replaced the checks with a simple try/except.
Pycharm will give you errors if the code is not pep8 compliant (you can test by putting more than 1 line in function in the class), it also checks for compliance with python versions, currently the code works on 2.7 to 3.5 (except 3.0 and 3.1 because of argparse) (the ones I set in pycharm to test for), if the code is not compliant it will tell you which version it will not work on (for example the StringIO on line 13 is not compliant with 3.0 to 3.5, so the line 15 fixes that).
But pep8 also states max line length IIRC. That rule is definitely violated here. ;) For now, I'll stick to good old vim + python-mode + Syntastic. :P
Yeah, if you hover over line 34 it complains about that, but this is 2016 (most of us have 1080p monitor, or even 4K hehe), 80 lines is a bit short, I usually go for 120 lines. Although if you want to change it, I wouldn't mind :)
Well, the rules are the rules. And I am an exception then. My monitor is only 1024p. Actually it is a 19" 5:4. I did have to scroll sideways, a lot. :(
Ah, yeah, I suppose it also makes sense on github, I think they are at around 100 lines wide?
Don't know, but I think 80, or 79, is good, really. There is a reason for this line length. It is just more readable. Look how many mailing lists still demand this.
You can zoom to 300%, however, to fill your screen width. ;)
But, honestly, if you do a lot of reading, too much horizontal eye movement is tiresome
Going back to the file / which discussion.
I'm not sure how each program we require handles exit codes when no arguments are passed.
If they all return non 0, we can test for that.
For file, we can maybe try checking the mime type instead.
Simply trying to run an non-existent program should raise an exception. I checked by just mispelling "which" as "witch" before my last commit.
Yeah, it's checking the exit codes, and raises an exception.
It cannot get an exit code of a program that is not there. The exception comes from the OS, as the OSError also seems to imply.
Ah, my bad I was reading the line 70 (the subprocess call).
Well, that is the one that will either make the program run with null output, but no exception, or there will be an exception, if "binary" is not executable, i.e. not in PATH.
Yeah.
Will check on mime types now so we can remove file.
Now, I'm unsure how to go forward with how to get the tags and merge the tags we have, it's something I thought about today but couldn't wrap my head around.
Will check on mime types now so we can remove file.
I think that is not necessary. Why not simply use, say, mkvinfo. It exits 2, IIRC, when there is no valid matroska file.
BTW, thx for the pep8 change. I appreciate it very much. :)
Now, I'm unsure how to go forward with how to get the tags and merge the tags we have, it's something I thought about today but couldn't wrap my head around.
Yeah, thinking about that too. Let's check what we got.
Is using mkvinfo faster than mimetypes?, I'll try in a test file for fun.
We get mkvmerge identification info in json. That gives us some way to map Track IDs or stream IDs to their respective UIDs.
Is using mkvinfo faster than mimetypes?, I'll try in a test file for fun.
Don't know, just figured it's easier, and since we need it anyway, I thought why not. But I might be thinking too much in shell terms there. I once had find check for mime types to make sure all files it finds are videos, regardless of file extension. That took ages to run. All because I was too lazy to make a list of all known extensions. ;)
Oh, mimetypes is massively faster, quite shocked really, it's roughly 76.7 times faster.
On 100 runs of mimetypes on a file it takes 0.03 seconds to process.
On 100 runs of mkvinfo 2.51 seconds.
Example script / output : https://gist.github.com/kevinlekiller/250d8482b60c5fb8b4af4ad89dc0d443
Although I get what you mean, when we eventually use mkvinfo it will error if it's not matroska, but for now I think mimetypes is fine.
Yeah, this might be due to the overhead incurred by running subprocess()
Also you are parsing the output of mkvinfo, right?
In the test script? It's just ignoring it.
but it retrieves the output, which may not be necessary, all I thought of was the exit code. Output can go to null.
subprocess.call(["mkvinfo", file], stdout=devnull, stderr=devnull)
Very similar to the check if the program runs.
Ah alright, will test that.
Hmm, well that requires creating a "devnull"
It is faster, but 2.34 seconds instead of 2.44
def mkvinfo2():
import subprocess
import os
global file
devnull = open(os.devnull, 'w')
subprocess.call(["mkvinfo", file], stdout=devnull, stderr=devnull,)
devnull.close()
Yeah, I guess, the OS overhead just kills it.
If we could find a way to make those subprocess calls faster it would help for the other commands we run also.
BTW, why do you do the import inside the function?
Oh it was to keep it fair, for the test.
I wouldn't bother with that just now. I think this is the wrong picture. The runtime of bs1770gain overwhelms everything else.
Haha, yeah, I was looking for ways to speed that up.
I tried extracting the audio, then running bs1770gain, but it's just as slow.
While running other replaygain programs on the same audio file is MUCH faster.
Well, I don't think there is much difference, but we only import once but will check many times. So actually it would be fairer to only do the imports once.
Yeah, the idea was just to run 1 iteration, but the default on timeit is 100000 iterations.
(with 1 iteration it at least compares to if you would run on 1 file with mkvrg.py)
Yeah bs1770gain might be slow, but that is also due to true peak. That requires conversion to 24 bit audio, which is done in sox I think. But that and the algortihm is kind of slow.
while you are timing maybe just check that for my sake. ;)
on timed run of bs1770gain -p vs bs1770gain -r
Hmm, maybe we should add an option for true peak then, since it's not used in mpv.
Oh, it is
I think the man page is just misleading or plain wrong.
try mpv --af=volume=replaygain-track:replaygain-clip=no
and increase verbosity. You'll see the is an extra line: [volume] ...with clipping prevention: 0.444871