mlt icon indicating copy to clipboard operation
mlt copied to clipboard

Allow outputing frame audio peak in dB

Open j-b-m opened this issue 2 years ago • 3 comments

in Kdenlive we use the "audiolevel" filter to get the peak audio level for each track, using the _audio_level.<N> property. With this change, the filter can now optionally output the peak audio level in dB instead of a percentage, making it easier to use in vu-meter. No changes were done on the existing functionnalities

j-b-m avatar Sep 16 '22 18:09 j-b-m

On Freitag, 16. September 2022 21:42:26 CEST Dan Dennedy wrote:

Thanks for the review.

@ddennedy requested changes on this pull request.

  • type: integer
  • minimum: 0
  • maximum: 1

Use boolean here and get rid of minimum and maximum. To do so, you need to change the schema_version above to "7.0". And please update the iec_scale parameter to match.

Done. {

  • 		level = 1.0;
    
  • 		break;
    
  • 		level = 20 * log10( 
    

(double)peakVal / (double)INT16_MAX );

Use AMPTODBFS() here.

Done.

@@ -61,6 +61,7 @@ static int filter_get_audio( mlt_frame frame, void **buffer, mlt_audio_format *f mlt_properties filter_props = MLT_FILTER_PROPERTIES( filter );

int iec_scale = mlt_properties_get_int( filter_props, "iec_scale" );

  • int peak = mlt_properties_get_int( filter_props, "peak" );

Why "peak" instead of something about dbFS unit of measure? Also, the current code is already supposed to be like a peak meter when iec_scale=1 as described [here on wikipedia](https://en.wikipedia.org/wiki/Peak_programme_meter#IEC_60268-18_ Digital_PPM). Looking at the new code below, this looks like a different, non-averaging implementation of peak. Is it true peak?

Sure the parameter name was not the best. I now changed it to "dbpeak". And in fact, the code was borrowed from Shotcut's audio peak scope:

https://github.com/mltframework/shotcut/blob/master/src/widgets/scopes/ audiopeakmeterscopewidget.cpp#L46

I am not an expert in audio scales, but when playing a standardized, stable sine test tone with 1 kHz and -9 dB, using iec_scale=1, the _audio_level property is set to 0.83 when using my updated code (same as shotcut), the _audio_level property is set to -8.99.

See also the shotcut commit referencing issues in audiolevel filter: https://github.com/mltframework/shotcut/commit/ f5928ed3dcdd57dcbb7f311cadfb1224ce0b034a

Best regards,

Jean-Baptiste

j-b-m avatar Sep 17 '22 08:09 j-b-m

What should the user expect if they set iec_scale=true and dbpeak=true? Will we keep adding more boolean parameters as other people request different calculations? I wonder if we should have one parameter with three options: percent, iec_scale, peak. We could deprecate the iec_scale parameter.

Also, what if someone wants both iec_scale and peak at the same time? Would it make sense to always calculate all three values? If you reformat the code to calculate all three values during one loop through the samples, the extra computation would be trivial. But we would have to decide what to name all three value properties on the frame. Maybe that is too complicated.

bmatherly avatar Sep 17 '22 12:09 bmatherly

What should the user expect if they set iec_scale=true and dbpeak=true?

Do bear in mind that "true" and "false" are not accepted literal values for a a "boolean" parameter, and that metaschema.yaml says only 0 or 1. This is because the metadata is documentation oriented, but the implementation in mlt_properties uses int. In any case, for a logical reading in a comment it is fine. I just want to clear that up since I raised changing the metadata parameter type.

With that said, the question is interesting because you could make a very small change to this PR to support that combination.

if (iec_level)
    level = IEC_Scale(level);

ddennedy avatar Sep 17 '22 21:09 ddennedy

LGTM

bmatherly avatar Sep 27 '22 00:09 bmatherly