desbma / r128gain

Fast audio loudness scanner & tagger (ReplayGain v2 / R128)
GNU Lesser General Public License v2.1
170 stars 9 forks source link

Don't ignore files provided on the command-line in recursive mode #41

Closed xeruf closed 1 year ago

xeruf commented 1 year ago

Thanks for the fantastic tool!

One little issue:

❯ r128gain -r --skip-tagged Alles\ tanzt\ _\ YADA\ Worship\ _\ Piano\ Tutorial\ \&\ Mainstage\ Patch.mp4
❯ r128gain --skip-tagged Alles\ tanzt\ _\ YADA\ Worship\ _\ Piano\ Tutorial\ \&\ Mainstage\ Patch.mp4
File 'Alles tanzt _ YADA Worship _ Piano Tutorial & Mainstage Patch.mp4' already has a track gain tag, skipping track gain scan
File 'Alles tanzt _ YADA Worship _ Piano Tutorial & Mainstage Patch.mp4': loudness = SKIPPED, sample peak = SKIPPED

I would expect it to also process provided files in recursive mode, like standard tools such as cp and mv.

desbma commented 1 year ago

I don't see how that would not be extremely counter intuitive. The recursive mode and logic to exclude already tagged files are different things, I don't see the rationale for negating the second when the first is set.

xeruf commented 1 year ago

--skip-tagged is unrelated in this command, I just used it for demonstration because I did not want to retag the file, imagine it not being there ;)

the point is that the first and second command should do exactly the same thing in this case

desbma commented 1 year ago

Oh I see, I misunderstood what you meant by "skipped". The issue with that, is that when computing album gain, in non recursive mode all files passed on the command line are assumed to be from the same album, and in recursive mode, files will be considered from the same album only if they are in the same directory. Passing both files and directories would require having an inconsistent hybrid logic to find which file belongs to which album, which I would like to avoid.

xeruf commented 1 year ago

so maybe you can reject the combination -r -a if files are given, but proceed otherwise?

when computing album gain, in non recursive mode all files passed on the command line are assumed to be from the same album

Also, this should be documented, I don't think it is.

desbma commented 1 year ago

Also, this should be documented, I don't think it is.

It probably should, but it seems like the most natural behavior (subjective, I know) and also the same behavior as vorbisgain, mp3gain, wvgain, etc.

I just checked how vorbisgain behaves when mixing files and directories with album gain and recursive mode:

$ tree
 .
├──  a.ogg
├──  b
│   └──  b.ogg
└──  c
    └──  c2
        ├──  c1.ogg
        └──  c2.ogg

$ vorbisgain -ard a.ogg b/b.ogg c
Analyzing files...

   Gain   |  Peak  | Scale | New Peak | Track
----------+--------+-------+----------+------
 -9.30 dB |  37451 |  0.34 |    12837 | a.ogg
 -6.15 dB |  33499 |  0.49 |    16502 | b/b.ogg

Recommended Album Gain: -9.15 dB

Processing directory 'c/c2':
Analyzing files...

   Gain   |  Peak  | Scale | New Peak | Track
----------+--------+-------+----------+------
 -9.97 dB |  40066 |  0.32 |    12714 | c1.ogg
-10.62 dB |  38026 |  0.29 |    11197 | c2.ogg

Recommended Album Gain: -10.17 dB

So it considers all given files from the same album regardless of their directories, unlike files found by recursing into given directories. I find it confusing and I would rather not do that.

so maybe you can reject the combination -r -a if files are given, but proceed otherwise?

Honestly I prefer the more consistent current behavior. It covers the most common use cases (tag a bunch of files, tag a full library). If you want to somehow mix both, it is up to you to resolve the album ambiguity by running it twice (once on individual files, once on a tree) if needed.

xeruf commented 1 year ago

How is it consistent to just do nothing when combining -r with a provided file? I still find it highly confusing.

desbma commented 1 year ago

I think I will change it to throw an error if files instead of directories are given in recursive mode.

It may not be what you would prefer, but at least it removes the "silently skips files" issue.

xeruf commented 1 year ago

yes, not my preferred way to solve it but a valid alternative - explicit errors over silent failure :)

desbma commented 1 year ago

https://github.com/desbma/r128gain/commit/024f649e194a99e6eba6be08b3518409a96a2076