-
Notifications
You must be signed in to change notification settings - Fork 16
Also report peaks as sample peak relative amplitude #116
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
base: master
Are you sure you want to change the base?
Conversation
|
I don't understand this at all. Precise? calculated from ebur128 dBFS? And they match? |
|
I changed the ebur128 ffmpeg filtergraph configuration string so that it calculates both sample peak and true peak. "ebur128=peak=true+sample,anullsink"It gives these in terms of dB. To convert those values to relative amplitudes I used the formula True peak in dB is what we currently have in the log and what I'd like to add is the value I labeled "Sample peak relative amplitude (precise)" since this is reported by other rippers and is commonly used as a crude "fingerprint" of a disc to identify distinct masterings. I'm unsure of how to nicely report both of these in the log because of the differing units. I was thinking maybe report both of them as relative amplitude. So from the example above, we'd report just those last two values as something like this: But I wanted to get your opinion. |
12b7a76 to
0c09287
Compare
| cyanrip_log(ctx, 0, " Sample peak relative amplitude (calculated from ebur128 dBFS):\n"); | ||
| cyanrip_log(ctx, 0, " Peak: %f\n\n", track_sample_peak_rel_amp_ebu); | ||
| cyanrip_log(ctx, 0, " Sample peak relative amplitude (precise):\n"); | ||
| cyanrip_log(ctx, 0, " Peak: %f\n\n", track_sample_peak_rel_amp_precise); | ||
| cyanrip_log(ctx, 0, " True peak relative amplitude (calculated from ebur128 dBFS):\n"); | ||
| cyanrip_log(ctx, 0, " Peak: %f\n\n", track_true_peak_rel_amp_ebu); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very random place to put this. Put it where the other log printouts are. And store the fields in the per-track struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually is next to another log printout. The cyanrip_finalize_encoding() call right above it causes the preceding part of the log to be printed by ffmpeg. During that function call, this part of the log gets printed:
Summary:
Integrated loudness:
I: -6.5 LUFS
Threshold: -16.7 LUFS
Loudness range:
LRA: 6.6 LU
Threshold: -26.6 LUFS
LRA low: -11.4 LUFS
LRA high: -4.8 LUFS
Sample peak:
Peak: -0.3 dBFS
True peak:
Peak: -0.3 dBFS
As for storing the fields, will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the code in ffmpeg that writes that part of the log: https://github.com/FFmpeg/FFmpeg/blob/master/libavfilter/f_ebur128.c#L1055
Maybe this would be a good opportunity to switch over to writing that part of the log ourselves? The current situation is that we have a dependency directly writing to our log as part of a deinitialization function, which makes for very confusing code and means we don't fully control our own log, e.g. we're forced to mimic the ffmpeg log style so that the indentation lines up and the log looks cohesive.
|
The names are absolutely terrible.
Then why keep the ebur128 dBFS? |
lol. I agree. I found these tricky to give good names to and I didn't mean to suggest that this get pulled with those exact names still there. I wanted to get your opinion on what to write to the log so I opened the pull request with the guts still hanging out.
Yeah, the sample peak amplitude calculated from ebur128 dBFS is junk and can be tossed, but the true peak amplitude calculated from ebur128 dBFS may still be relevant so for now I kept its sample peak counterpart around just for the sake of symmetry. Again, things are in a very guts-hanging-out state because I wanted to get your opinion on what to include in the log before condensing it. |
0c09287 to
2c7ba0f
Compare
|
What about this for the log format? It's a little clumsy but it wouldn't require touching the way that ffmpeg writes to the log. For reimplementing the ffmpeg printouts ourself, I'm not sure if there's a way to access the two values labeled "Threshold" since they're not in the in the |
…ation from ebur128 dBFS and by iteration over samples.
e2a25cd to
d2383a6
Compare
By "sample peak relative amplitude" what I mean is:
sample peak: value is taken from an actual sample
relative amplitude: as a fraction of the maximum possible value instead of dBFS
Providing this number allows comparison with EAC and XLD logs and this number is commonly used as a basic "fingerprint" to help determine if a disc is a distinct mastering.
This is almost in good shape to be merged although you'll need to tell me how you'd like this reported in the logs. Right now it's very programmer-oriented:
I started off just calculating from the ebur128 dBFS, but that result is sometimes slightly off compared to XLD, presumably due to rounding error. I then added "precise", which is a simple iteration over all the samples, which as far as I've seen always matches XLD.
I'm not quite sure how best to tidy up the log. The two values that matter here are one of true peaks and
Sample peak relative amplitude (precise). Maybe show the values for onlyTrue peak relative amplitude (calculated from ebur128 dBFS)andSample peak relative amplitude (precise)and clean up their names?Thoughts?